From owner-freebsd-scsi@FreeBSD.ORG Tue Oct 25 18:43:05 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 C0CF9106564A for ; Tue, 25 Oct 2011 18:43:05 +0000 (UTC) (envelope-from dgilbert@interlog.com) Received: from smtp.infotech.no (smtp.infotech.no [82.134.31.41]) by mx1.freebsd.org (Postfix) with ESMTP id 6A2B08FC14 for ; Tue, 25 Oct 2011 18:43:05 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp.infotech.no (Postfix) with ESMTP id 7956B2041B9 for ; Tue, 25 Oct 2011 20:26:50 +0200 (CEST) X-Virus-Scanned: by amavisd-new-2.6.6 (20110518) (Debian) at infotech.no Received: from smtp.infotech.no ([127.0.0.1]) by localhost (smtp.infotech.no [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rkHvK3qohyAG for ; Tue, 25 Oct 2011 20:26:48 +0200 (CEST) Received: from [192.168.48.66] (ip-191.55.99.216.dsl-cust.ca.inter.net [216.99.55.191]) by smtp.infotech.no (Postfix) with ESMTPA id 5E1782041B6 for ; Tue, 25 Oct 2011 20:26:48 +0200 (CEST) Message-ID: <4EA6FF66.1000508@interlog.com> Date: Tue, 25 Oct 2011 14:26:46 -0400 From: Douglas Gilbert User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.23) Gecko/20110922 Thunderbird/3.1.15 MIME-Version: 1.0 To: freebsd-scsi@freebsd.org References: <02B04E968B8648CC83F274B32090B937@multiplay.co.uk> <20111024171039.GA39194@nargothrond.kdm.org> <7E5239236AA04319B4C090E5F199DC64@multiplay.co.uk> <20111025173148.GA93047@nargothrond.kdm.org> In-Reply-To: <20111025173148.GA93047@nargothrond.kdm.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: Looking for a committer for cam fixes / enhancements X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: dgilbert@interlog.com List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2011 18:43:05 -0000 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" >> >>> 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.