From owner-freebsd-scsi@FreeBSD.ORG Tue Oct 25 17:31:49 2011 Return-Path: Delivered-To: freebsd-scsi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 643091065670; Tue, 25 Oct 2011 17:31:49 +0000 (UTC) (envelope-from ken@kdm.org) Received: from nargothrond.kdm.org (nargothrond.kdm.org [70.56.43.81]) by mx1.freebsd.org (Postfix) with ESMTP id 24EA18FC0A; Tue, 25 Oct 2011 17:31:48 +0000 (UTC) Received: from nargothrond.kdm.org (localhost [127.0.0.1]) by nargothrond.kdm.org (8.14.2/8.14.2) with ESMTP id p9PHVmuA008070; Tue, 25 Oct 2011 11:31:48 -0600 (MDT) (envelope-from ken@nargothrond.kdm.org) Received: (from ken@localhost) by nargothrond.kdm.org (8.14.2/8.14.2/Submit) id p9PHVmKE008069; Tue, 25 Oct 2011 11:31:48 -0600 (MDT) (envelope-from ken) Date: Tue, 25 Oct 2011 11:31:48 -0600 From: "Kenneth D. Merry" To: Steven Hartland Message-ID: <20111025173148.GA93047@nargothrond.kdm.org> References: <02B04E968B8648CC83F274B32090B937@multiplay.co.uk> <20111024171039.GA39194@nargothrond.kdm.org> <7E5239236AA04319B4C090E5F199DC64@multiplay.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7E5239236AA04319B4C090E5F199DC64@multiplay.co.uk> User-Agent: Mutt/1.4.2i Cc: freebsd-scsi@freebsd.org, Eygene Ryabinkin Subject: Re: Looking for a committer for cam fixes / enhancements X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2011 17:31:49 -0000 On Tue, Oct 25, 2011 at 00:45:05 +0100, Steven Hartland wrote: > > ----- Original Message ----- > From: "Kenneth D. Merry" > > >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@FreeBSD.ORG