Looking for a committer for cam fixes / enhancements

[Douglas Gilbert]

On 11-10-25 01:31 PM, Kenneth D. Merry wrote:
> On Tue, Oct 25, 2011 at 00:45:05 +0100, Steven Hartland wrote:
>>
>> ----- Original Message -----
>> From: "Kenneth D. Merry"<ken at freebsd.org>
>>
>>> Thanks for doing these!  They seem like they would be very useful.
>>> Hopefully, once we get trim support plumbed all the way down, it
>>> won't be necessary to issue the erase manually.
>>
>> Indeed, would so love to see trim added to zfs :)
>>
>>> I do have a few comments:
>>>
>>> - The patches should be generated against head, since they would be
>>>   committed there first and merged back.  (They don't apply cleanly to
>>>   head.)
>>>
>>> - There are a number of style issues in the patches:
>>> - Lines longer than 80 characters
>>> - Spaces/formatting problems (e.g. at the beginning of
>>>   atasecurity_erase()).
>>> - The prevailing style of the file isn't followed for line
>>>   continuations.  (It isn't always KNF, either.)
>>>
>>> - I'm not really a fan of getopt_long.  There are lots of password
>>>   arguments, how about turning those into '-p foopasswd=bar' instead?
>>>
>>> Anyway, if you could, please address those things and send me the diffs.
>>
>> Thanks for the feedback. I will get them updated as per comments as
>> soon as I can.
>>
>> Could you give me some points on the things you spotted weren't "KNR"
>> I've tried to stick with what I saw as the current formatting but clearly
>> missed something's ;-)
>
> To match the rest of the file this:
>
> static int
> atasecurity_set_password(struct cam_device *device, union ccb *ccb, int retry_count,
> 	u_int32_t timeout, struct ata_security_password *pwd, int quiet)
> {
>
> Should look like this:
> static int
> atasecurity_set_password(struct cam_device *device, union ccb *ccb,
> 			 int retry_count, u_int32_t timeout,
> 			 struct ata_security_password *pwd, int quiet)
> {
>
> And this:
>
>          cam_fill_ataio(&ccb->ataio,
> 		    retry_count,
> 		    NULL,
> 		    /*flags*/CAM_DIR_OUT,
> 		    MSG_SIMPLE_Q_TAG,
> 		    /*data_ptr*/(u_int8_t *)pwd,
> 		    /*dxfer_len*/sizeof(struct ata_security_password),
> 		    timeout);
>
> Shoud look like this:
>
>          cam_fill_ataio(&ccb->ataio,
> 		       retry_count,
> 		       NULL,
> 		       /*flags*/CAM_DIR_OUT,
> 		       MSG_SIMPLE_Q_TAG,
> 		       /*data_ptr*/(u_int8_t *)pwd,
> 		       /*dxfer_len*/sizeof(struct ata_security_password),
> 		       timeout);
>
>
> Everything that exceeds 80 columns should not exceed that.  Make sure the
> tab stop in your editor is set to 8 when editing code.  That helps uncover
> things like this (in atasecurity()):
>
> 	while ((c = getopt_long(argc, argv, combinedopt, combinedoptlong, NULL)) != -1) {
>          switch(c){
>          case 'f':
> 			action = ATA_SECURITY_ACTION_FREEZE;
> 			actions++;
> 			break;
>
> 		case 'r':
>
> With a tabstop set to 4, that particular space/tab problem "hides".
>
>> Not really a fan myself of mixing in getopt_long either but if beats the
>> current use of totally random / meaningless letters for options if we
>> stuck with short opts. Add that to how dangerous the options are and I
>> think forcing long opts is the right move, what do others think?
>
> I'm not suggesting short options, but a slightly different approach.
> Instead of --option, or -r, use -o foopassword=blah.
>
> It might be interesting to do this:
>
> cd src
> find bin sbin usr.sbin usr.bin -name "*.c" -print |xargs grep getopt_long
>
> The list of programs that use getopt_long() is very short, and almost all
> of them are either GNU programs, or maintaining compatibility with GNU
> programs (e.g. tar, cpio)

I can think of about 60 utilities that I have written,
all open source and running on FreeBSD, that use
getopt_long(). IMO the "-o foopassword=blah" style
options don't scale well (and remind me of misplaced
dd style "operands" (e.g. 'bs=512 of=/dev/null')).

About the only OS that gave me problems with getopt_long()
was Tru64 and it is becoming a distant memory.

Doug Gilbert


Here is an example:

$ sg_format --help
usage: sg_format [--cmplst=0|1] [--count=COUNT] [--dcrt] [--early]
                  [--fmtpinfo=FPI] [--format] [--help] [--long] [--pfu=PFU]
                  [--pie=PIE] [--pinfo] [--resize] [--rto_req] [--security]
                  [--six] [--size=SIZE] [--verbose] [--version] [--wait]
                  DEVICE
   where:
     --cmplst=0|1
       -C 0|1        sets CMPLST bit in format cdb (default: 1)
     --count=COUNT|-c COUNT    number of blocks to report after format or
                               resize. With format defaults to same as current
     --dcrt|-D       disable certification (doesn't verify media)
     --early|-e      exit once format started (user can monitor progress)
     --fmtpinfo=FPI|-f FPI    FMTPINFO field value (default: 0)
     --format|-F     format unit (default: report current count and size)
     --help|-h       prints out this usage message
     --long|-l       allow for 64 bit lbas (default: assume 32 bit lbas)
     --pfu=PFU|-P PFU    Protection Field Usage value (default: 0)
     --pie=PIE|-q PIE    Protection Information Exponent (default: 0)
     --pinfo|-p      set upper bit of FMTPINFO field
                     (deprecated use '--fmtpinfo=FPI' instead)
     --resize|-r     resize (rather than format) to COUNT value
     --rto_req|-R    set lower bit of FMTPINFO field
                     (deprecated use '--fmtpinfo=FPI' instead)
     --security|-S    set security initialization (SI) bit
     --six|-6        use 6 byte MODE SENSE/SELECT to probe device
                     (def: use 10 byte MODE SENSE/SELECT)
     --size=SIZE|-s SIZE    bytes per block, defaults to DEVICE's current
                            block size. Only needed to change current block
                            size
     --verbose|-v    increase verbosity
     --version|-V    print version details and exit
     --wait|-w       format command waits until format operation completes
                     (default: set IMMED=1 and poll with Test Unit Ready)

	Example: sg_format --format /dev/sdc

This utility formats or resizes SCSI disks.
WARNING: This utility will destroy all the data on DEVICE when
	 '--format' is given. Check that you have the correct DEVICE.