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