From owner-svn-src-projects@FreeBSD.ORG Sun Mar 18 03:57:46 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 66016106566B; Sun, 18 Mar 2012 03:57:46 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail36.syd.optusnet.com.au (mail36.syd.optusnet.com.au [211.29.133.76]) by mx1.freebsd.org (Postfix) with ESMTP id 961028FC08; Sun, 18 Mar 2012 03:57:45 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail36.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q2I3vZTs029531 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 18 Mar 2012 14:57:36 +1100 Date: Sun, 18 Mar 2012 14:57:35 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pawel Jakub Dawidek In-Reply-To: <20120317215156.GJ1340@garage.freebsd.pl> Message-ID: <20120318142037.P1308@besplex.bde.org> References: <201203171710.q2HHAFiq079651@svn.freebsd.org> <20120317215156.GJ1340@garage.freebsd.pl> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-projects@freebsd.org, Grzegorz Bernacki , 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: Sun, 18 Mar 2012 03:57:46 -0000 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