svn commit: r233091 - in projects/nand: sbin/fdisk sys/sys

[Bruce Evans]

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