On Sun, Jul 22, 2007 at 07:52:36AM +0200, Jens Axboe wrote:
> On Sun, Jul 22 2007, Andi Kleen wrote:
> > Lars Ellenberg <[email protected]> writes:
> > >
> > > Jens, Andrew, anyone: please review,
> > > and give me advice how to proceed from here.
> >
> > The standard procedure would be to post all the source code in logical
> > pieces on the list for review. Then iterate until all comments are
> > addressed.
>
> Yep, cleanup the style issues (that make sense) from checkpatch and then
> psot as a series of patches that can be reviewed. Linking to a git tree
> wont get you very far.
it got me far enough, for the first try, anyways :-)
I did not spam the lkml with patches, and still got some very useful
advice (no idea how I could overlook the checkpatch.pl complaints).
If each patch of a series needs to compile and work,
there will probably only one 17kB patch...
it is difficult to split one module into a series of patches.
Or am I missing something?
may I bother you again to comment on this, please:
I am now down to
5 CHECK: memory barrier without comment
these are directly connected with our homegrown kernel thread stuff.
will vanish as soon as we convert to kthread.h API.
4 CHECK: spinlock_t definition without comment
3 CHECK: Use #include <linux/uaccess.h> instead of <asm/uaccess.h>
3 CHECK: if this code is redundant consider removing it
2 CHECK: Use #include <linux/types.h> instead of <asm/types.h>
need to check those, still.
72 ERROR: braces {} are not necessary for single statement blocks
one branch needs them, the othe does not.
what now? CodingStyle and checkpatch.pl disagree.
13 ERROR: no space between function name and open parenthesis '('
this is about our ERR_IF() macro...
suggestions, anyone?
do need I to explicitly write these out?
8 ERROR: Macros with multiple statements should be enclosed in a do - while loop
1 ERROR: Macros with complex values should be enclosed in parenthesis
these are "netlink packet definition language" in drbd_nl.h,
which is sort of clean, and preprocessor magic in drbd_nl.c.
suggestions how to handle this cleanly,
without making it more ugly?
autogenerate code by other means?
write it out by hand, and lose the nice and clean drbd_nl.h?
1 ERROR: Don't use kernel_thread(): see Documentation/feature-removal-schedule.txt
yes. working on that.
will take some days, though.
1 ERROR: Missing Signed-off-by: line(s)
94 WARNING: declaring multiple variables together should be avoided
int snr, enr;
does this really need to become two lines?
33 WARNING: line over 80 characters
hmmm. get more ugly...
probably need some helper functions and temp variables?
4 WARNING: multiple assignments should be avoided
x = y = 0;
is sometimes a convenient initialization.
you don't like it?
--
: Lars Ellenberg Tel +43-1-8178292-0 :
: LINBIT Information Technologies GmbH Fax +43-1-8178292-82 :
: Vivenotgasse 48, A-1120 Vienna/Europe http://www.linbit.com :
__
please use the "List-Reply" function of your email client.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
