Re: [DRIVER SUBMISSION] DRBD wants to go mainline

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/22/07, Lars Ellenberg <[email protected]> wrote:
On Sun, Jul 22, 2007 at 07:52:36AM +0200, Jens Axboe wrote:
> [...]
> 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?

If not too much bother, you could even present the patchset in a way
that reflects how the driver code evolved to its present state in v8.0.4.

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.

Submit a patch to checkpatch.pl (or preferably CodingStyle, I dislike
with the way it blatantly disregards K&R convention in this matter :-)

13 ERROR: no space between function name and open parenthesis '('
    this is about our ERR_IF() macro...
    suggestions, anyone?

There shouldn't be any space between function/macro name and open
parenthesis. However, there *must* be a space between C language
keyword (if, while, for) and open parenthesis.

    do need I to explicitly write these out?

Yup, do consider that. Also consider making them functions. Macros
are _generally_ evil and *always* horrible.

 8 ERROR: Macros with multiple statements should be enclosed in a do - while loop

You don't want to break if-else constructs.

 1 ERROR: Macros with complex values should be enclosed in parenthesis

I'm not sure know when / why checkpatch.pl reports this. I don't want to
pull your tree so can't check for myself, but note some obvious rules:

1. Macro definition must always include parenthesis around the entire
definition (unless it's a do { somehting; } while (0) kind of macro).
2. Use parenthesis around wherever it uses the passed argument(s).

    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. Open-code it if it is trivial enough and there's no point
obfuscating anything.
2. Consider making the macro a function. Probably inline, most probably not.
3. Macros must NOT evaluate the passed argument twice -- consider the
possible damages of someone who doesn't know it's a macro and therefore
passing in an expression that has side-effects (*boom, crash*).

 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)

Patches submitted to LKML should conform to guidelines in:

* Documentation/SubmittingPatches in kernel sources
* http://www.zipworld.com.au/~akpm/linux/patches/stuff/tpp.txt

94 WARNING: declaring multiple variables together should be avoided
    int snr, enr;
    does this really need to become two lines?

No, if these are some unimportant temporary/automatic variables in
some function.

Yes, if they are members of some struct (also comment them in
this case -- in fact give full kernel-doc style comments).

33 WARNING: line over 80 characters
    hmmm. get more ugly...
    probably need some helper functions and temp variables?

Yes, do consider breaking functions that go significantly beyond
80 lines into smaller ones. If it's just 4-5 columns beyond 80, and
breaking the line would actually hurt readability, don't bother.

 4 WARNING: multiple assignments should be avoided
    x = y = 0;
    is sometimes a convenient initialization.
    you don't like it?

Yes, we don't appreciate such usage at all. Please fix this.

Satyam
-
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]
  Powered by Linux