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

[Grzegorz Bernacki]

W dniu 2012-03-19 02:57, Grzegorz Bernacki pisze:
> 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.
>

Hi Bruce,

All changes in this file has been removed.

thanks,
Grzesiek