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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
