Re: Preparing btier for kernel inclusion

[walter harms <wharms@bfs.de>]

IMHO,
there is a indent line in the kernel styleguide.

re,
 wh


Am 12.02.2013 11:09, schrieb Mark Ruijter:
> 
> Hi Dan,
> 
> Thanks for your elaborate and fast response.
> I will change things as suggested and get back afterwards.
> 
> On quick question about indenting:
> Is there a set of options to indent that can produce indented code that
> is acceptable?
> Like : indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs -cp1 *.c
> 
> Or is manual labour a requirement? ;-)
> 
> Best regards,
> 
> Mark
> 
> -- 
> On 02/12/2013 10:47 AM, Dan Carpenter wrote:
>> This is staging quality code.  (Not very good).  The staging tree is
>> closed for the 3.9 window so it couldn't make it in until 3.10.
>>
>> *) Clean up the indenting.
>> *) Run checkpatch.pl over this and fix the warnings.
>> *) Don't include .c files into other .c files.
>> *) Get rid of homemade print macros like TIERERR().  Use dev_err()
>>     or pr_err().
>> *) Delete all compat code with older kernels like
>>     #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,0,0)
>> *) Change Yoda code to the if (NULL = odinfo) to normal format
>>     if (!odinfo)
>>         return -ENOMEM;
>>     Or "if (NULL != dev->backdev[0]->blocklist)" should be:
>>     if (dev->backdev[0]->blocklist)
>>         memcpy( ...
>> *) Run Sparse and Smatch over the code.  This should complain about
>>     some poor error handling.  For example, printing "no memory"
>>     followed by a dereference instead of actual error handling.  Or
>>     in the ioctl() it returns with the lock held.
>>
>> *) Make magic numbers a define.
>>     /* Allow max 24 devices to be configured */
>>     devicenames = kmalloc(sizeof(char) * 26, GFP_KERNEL);
>>
>>     for (count = 0; count <= 25; count++) {
>>         devicenames[count] = 97 + count;
>>     }
>>
>>     Where does the 97 come from?  Also it's hella messy to use
>>     24, 25, and 26!  The for loop should be written in the normal
>>     way:
>>
>>     #define MAX_BTEIR_DEVS 26
>>
>>     for (i = 0; i < MAX_BTEIR_DEVS; i++) {
>>
>>     Btw, use "i" instead of "count" for the iterator.  Use "count"
>>     for counting.
>>
>>     Some of the magic numbers are just wrong:
>>     res = snprintf(buf, 1023, "%s\n", msg);
>>     1023 should have been PAGE_SIZE.  If the size argument to
>>     snprintf() is non-zero then snprintf() adds a NUL terminator.
>>     No one ever creates a 1023 char buffer.  Also the return value
>>     is the number of bytes that would have been printed if there
>>     were enough space (not counting the NUL terminator).  Consider
>>     using scnprintf().
>>
>> *) Use normal kernel style comments.
>>     /* single line comment */
>>
>>     /*
>>      * Multi line
>>      * comment.
>>      */
>>
>> *) Some of functions could use more comments.  What does
>>     allocated_on_device() return?  I would have assumed from the name
>>     that it returns a bool, but actually it returns a u64.
>>
>> *) It scares me that when list_for_each_safe() is used
>>     unnecessarily.  A lot of people assume it has to do with locking
>>     but it doesn't.  It's for when you remove a list item.  This is
>>     wrong:
>>
>>     list_for_each_safe(pos, q, &device_list) {
>>         count++;
>>     }
>>
>> *) Put a blank line between declarations and code.
>>
>> *) Use temp variables to make lines shorter:
>>
>> -    dev->backdev[count]->bitlistsize >> -        dev->backdev[count]->devmagic->bitlistsize;
>>
>> +    back = dev->backdev[count];
>> +    back->bitlistsize = back->devmagic->bitlistsize;
>>
>> *) Never return -1 as a error code.  Return proper error codes at
>>     every level.
>>
>> *) The TIER_DEREGISTER ioctl takes a kernel pointer from user space
>>     which is a bug.  It should be doing copy_from_user().
>>
>> There are a lot of other messy things about this code, but that
>> should be enough to get started.
>>
>> My advice is that people will take you a lot more seriously if you
>> clean it up and make a good first impression.  The block layer
>> people are crotchety.
>>
>> Also, when you send patches for review, send it as a patch which can
>> be reviewed without leaving the email client.  That way we can put
>> comments inline.
>>
>> regards,
>> dan carpenter
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>