From owner-svn-src-projects@FreeBSD.ORG Mon Mar 19 01:57:49 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 50E181065675; Mon, 19 Mar 2012 01:57:49 +0000 (UTC) (envelope-from gjb@semihalf.com) Received: from smtp.semihalf.com (smtp.semihalf.com [213.17.239.109]) by mx1.freebsd.org (Postfix) with ESMTP id CEF708FC24; Mon, 19 Mar 2012 01:57:48 +0000 (UTC) Received: from localhost (unknown [213.17.239.109]) by smtp.semihalf.com (Postfix) with ESMTP id 58724EC2E9; Mon, 19 Mar 2012 02:57:40 +0100 (CET) X-Virus-Scanned: by amavisd-new at semihalf.com Received: from smtp.semihalf.com ([213.17.239.109]) by localhost (smtp.semihalf.com [213.17.239.109]) (amavisd-new, port 10024) with ESMTP id 0y2YgR9XUQt1; Mon, 19 Mar 2012 02:57:39 +0100 (CET) Received: from [172.17.136.194] (adsl-64-175-228-30.dsl.sntc01.pacbell.net [64.175.228.30]) by smtp.semihalf.com (Postfix) with ESMTPSA id 8C4E4EBE32; Mon, 19 Mar 2012 02:57:37 +0100 (CET) Message-ID: <4F669290.8010109@semihalf.com> Date: Mon, 19 Mar 2012 02:57:36 +0100 From: Grzegorz Bernacki User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2 MIME-Version: 1.0 To: Bruce Evans References: <201203171710.q2HHAFiq079651@svn.freebsd.org> <20120317215156.GJ1340@garage.freebsd.pl> <20120318142037.P1308@besplex.bde.org> In-Reply-To: <20120318142037.P1308@besplex.bde.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: svn-src-projects@freebsd.org, Grzegorz Bernacki , Pawel Jakub Dawidek , src-committers@freebsd.org Subject: Re: svn commit: r233091 - in projects/nand: sbin/fdisk sys/sys X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Mar 2012 01:57:49 -0000 W dniu 2012-03-18 04:57, Bruce Evans pisze: > On Sat, 17 Mar 2012, Pawel Jakub Dawidek wrote: > >> On Sat, Mar 17, 2012 at 05:10:15PM +0000, Grzegorz Bernacki wrote: >>> Log: >>> Add ioctl and structures for accessing nand disk devices. >> >> Grzegorz, this is really wrong way to do it. Neither geom_dev nor >> geom_disk are the places to add NAND specific ioctls. >> ... > > This also has some style bugs. > >>> Modified: projects/nand/sys/sys/disk.h >>> ============================================================================== >>> >>> --- projects/nand/sys/sys/disk.h Sat Mar 17 16:40:15 2012 >>> (r233090) >>> +++ projects/nand/sys/sys/disk.h Sat Mar 17 17:10:14 2012 >>> (r233091) >>> @@ -116,6 +116,32 @@ void disk_err(struct bio *bp, const char >>> * This should be a multiple of the sector size. >>> */ >>> >>> +#define DIOCNOOBSIZE _IOR('d', 141, u_int) /* Get oob size */ >>> + /*- >>> + * Get the OOB area size of NAND flash device. >>> + */ >>> + > > In KNF, there is a tab after #define. This rule was followed by all > previous #define's in this file. > > In KNF, comments precede what they describe (except for short ones to > the right of definitions). This rule was broken by almost all previous > comments in this file, and the new code is mostly bug for bug compatible > with that :-). > > In KNF, there are usually no verbose descriptions on #define's like > this. Such comments belong in man pages. Such comments make the > actual definitions hard to see. Here the density of code:comments is > about 1/8. This rule was broken by almost all previous comments in > this file, and the new code is bug for bug compatible with that. Of > course, man pages are bug for bug compatible with this, and none even > mentions the newer DIOCG* ioctls. Even the ~10 year old DIOCGMEDIASIZE > ioctl is not documented in any man page. :-( > > The short comments to the right of the definitions are bogus when there > is a verbose one after the definitions. Most old definitions have this > bug. All new definitions have this bug. > > Comments beginning with "/*-" have special meanings. The "-" just > tells indent(1) not to reformat the comment. Its typical use is to > prevent formatting of comments that are hand-formatted with bullet > points. There is one such comment in this file, and, correctly, > only this one had the "-" markup. None of the new comments has fancy > formatting, so the "-" in all of them is bogus. "/*-" is also > conventionally abused to start copyright comments. Copyright comments > normally have bullet points, and even if they didn't then their vendor > might not want them reformatted, so they must start with "/*-" or > alternatively "/**" anyway, so the convention does little except > require correct style for them. > > >>> +#define DIOCNBLKSIZE _IOR('d', 142, u_int) /* Get block size */ >>> + /* - >>> + * Get the block size of NAND flash device. >>> + */ > > Here the "-" in the comment is just noise. > >>> + >>> +struct nand_oob_request { >>> + off_t offset; /* offset in bytes, page-aligned */ >>> + off_t length; /* length */ >>> + void * ubuf; /* buffer supplied by user */ >>> +}; > > In KNF, #defines are placed all together, without type declarations in > the middle. > > In KNF, struct members are only indented by 1 tab (or 1 tab plus 1 > space to line up after a '*') if possible. When this is done, comments > to the right of struct members are normally started in column 40. > > In KNF, the final '*' for pointers is attached to the name of the > variable > with no space between it and the name, not to the type with space[s] > between > it and the rest of the type. > >>> + >>> +#define DIOCNREADOOB _IOW('d', 143, struct >>> nand_oob_request) /* Read OOB area */ > > Now there is a tab after #define. > > In KNF, the maximum line length is 80. Here it is longer, with the help > of a verbose struct tag name. Too-long lines are especially bogus when > there is also a verbose comment about the same thing. Even if you don't > write man pages in the comments, a comment that won't fit in 80 columns > is sometimes needed, so it must be put on a separate line, but it is > hard to format the extra lines for this nicely. > >>> + /*- >>> + * Read page OOB area from NAND flash device. >>> + */ >>> + >>> +#define DIOCNWRITEOOB _IOW('d', 144, struct >>> nand_oob_request) /* Write OOB area */ > > Another with a correctly formatted #define and a too-long line. > >>> + /*- >>> + * Write page OOB area to NAND flash device. >>> + */ >>> + >>> #define DIOCGPHYSPATH _IOR('d', 141, char[MAXPATHLEN]) >>> /* >>> * Get a string defining the physical path for a given provider. > > Bruce Thanks for comments. We gonna cleanup this code. regards, grzesiek