Looking for a committer for cam fixes / enhancements

[Kenneth D. Merry]

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)

Ken
-- 
Kenneth Merry
ken at FreeBSD.ORG