Date: Mon, 19 Mar 2012 02:57:36 +0100 From: Grzegorz Bernacki <gjb@semihalf.com> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-projects@freebsd.org, Grzegorz Bernacki <gber@freebsd.org>, Pawel Jakub Dawidek <pjd@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r233091 - in projects/nand: sbin/fdisk sys/sys Message-ID: <4F669290.8010109@semihalf.com> In-Reply-To: <20120318142037.P1308@besplex.bde.org> References: <201203171710.q2HHAFiq079651@svn.freebsd.org> <20120317215156.GJ1340@garage.freebsd.pl> <20120318142037.P1308@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4F669290.8010109>