Date: Tue, 25 Oct 2011 14:26:46 -0400 From: Douglas Gilbert <dgilbert@interlog.com> To: freebsd-scsi@freebsd.org Subject: Re: Looking for a committer for cam fixes / enhancements Message-ID: <4EA6FF66.1000508@interlog.com> In-Reply-To: <20111025173148.GA93047@nargothrond.kdm.org> References: <02B04E968B8648CC83F274B32090B937@multiplay.co.uk> <20111024171039.GA39194@nargothrond.kdm.org> <7E5239236AA04319B4C090E5F199DC64@multiplay.co.uk> <20111025173148.GA93047@nargothrond.kdm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4EA6FF66.1000508>