From owner-freebsd-scsi@FreeBSD.ORG Fri Mar 9 18:49:32 2012 Return-Path: Delivered-To: freebsd-scsi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 90AF2106564A; Fri, 9 Mar 2012 18:49:32 +0000 (UTC) (envelope-from prvs=14151bc171=killing@multiplay.co.uk) Received: from mail1.multiplay.co.uk (mail1.multiplay.co.uk [85.236.96.23]) by mx1.freebsd.org (Postfix) with ESMTP id F080F8FC0A; Fri, 9 Mar 2012 18:49:31 +0000 (UTC) X-Spam-Processed: mail1.multiplay.co.uk, Fri, 09 Mar 2012 18:38:45 +0000 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on mail1.multiplay.co.uk X-Spam-Level: X-Spam-Status: No, score=-5.0 required=6.0 tests=USER_IN_WHITELIST shortcircuit=ham autolearn=disabled version=3.2.5 Received: from r2d2 ([188.220.16.49]) by mail1.multiplay.co.uk (mail1.multiplay.co.uk [85.236.96.23]) (MDaemon PRO v10.0.4) with ESMTP id md50018461258.msg; Fri, 09 Mar 2012 18:38:45 +0000 X-MDRemoteIP: 188.220.16.49 X-Return-Path: prvs=14151bc171=killing@multiplay.co.uk X-Envelope-From: killing@multiplay.co.uk Message-ID: <1DD65E0CC5144356A161732587F26266@multiplay.co.uk> From: "Steven Hartland" To: "Kenneth D. Merry" References: <02B04E968B8648CC83F274B32090B937@multiplay.co.uk> <20111024171039.GA39194@nargothrond.kdm.org> <7E5239236AA04319B4C090E5F199DC64@multiplay.co.uk> <20111025173148.GA93047@nargothrond.kdm.org> Date: Fri, 9 Mar 2012 18:38:35 -0000 MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset="iso-8859-1"; reply-type=original Content-Transfer-Encoding: 7bit X-Priority: 3 X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook Express 6.00.2900.5931 X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.6157 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: Fri, 09 Mar 2012 18:49:32 -0000 ----- Original Message ----- From: "Kenneth D. Merry" Sorry its been a quite a while, had no free time to look at this until now, but I have got a few questions if you would be so kind:- >> 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); > > So are you saying that all wrapped function definitions should have their wrapped lines indented via tabs (using ts=8) + spaces such that the wrapped lines align with the opening bracket of the arg list? If this is the case can you explain why as it means that all formatting needs to be done manually which seems needless? > Everything that exceeds 80 columns should not exceed that. Make sure the > tab stop in your editor is set to 8 when editing code. Again why in this day and age limit to 80? 132 I could just about understand but not 80 chars, does anyone really have a screen which can only display this amount of characters which they use to code on in this day and age especially given 8 char tab stops? The file already doesn't comply with this e.g. ata_bpack(ident_buf->revision, ident_buf->revision, sizeof(ident_buf->revision)); ata_btrim(ident_buf->serial, sizeof(ident_buf->serial)); ata_bpack(ident_buf->serial, ident_buf->serial, sizeof(ident_buf->serial)); In addition how do you deal with cases where a literal string e.g. if a printf is longer than 80 chars does that have to be split up? Overall this makes it harder to write compliant code (cant use standard editor formatting cabailities) as well as creating harder to read code, which all seems quite needless i.e. instead of:- error = atasecurity_unlock(device, ccb, retry_count, timeout, &pwd, quiet); I would have to have:- error = atasecurity_unlock(device, ccb, retry_count, timeout, &pwd, quiet); > 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) Sorry I don't mean to be a pain, but while this maybe what other programs are doing, are they also supporting as many options as camcontrol? Does it really make sense to have utils all do their own command line argument parsing which is required to process -o foopassword=blah, as suggested, instead of using the existing library designed to do this job for you? Seems like trying to stick with something that's no longer suitable, which will only cause supportability issues, simply because that's how its been done in the past? Regards Steve ================================================ This e.mail is private and confidential between Multiplay (UK) Ltd. and the person or entity to whom it is addressed. In the event of misdirection, the recipient is prohibited from using, copying, printing or otherwise disseminating it or any information contained in it. In the event of misdirection, illegible or incomplete transmission please telephone +44 845 868 1337 or return the E.mail to postmaster@multiplay.co.uk.