linux-kernel - Re: [DRIVER SUBMISSION] DRBD wants to go mainline

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070722145636.GA31276@one.firstfloor.org>
Date:	Sun, 22 Jul 2007 16:56:36 +0200
From:	Andi Kleen <andi@...stfloor.org>
To:	Lars Ellenberg <lars.ellenberg@...bit.com>
Cc:	Jens Axboe <jens.axboe@...cle.com>,
	Andi Kleen <andi@...stfloor.org>,
	Andrew Morton <akpm@...l.org>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [DRIVER SUBMISSION] DRBD wants to go mainline

On Sun, Jul 22, 2007 at 03:58:14PM +0200, Lars Ellenberg wrote:
> On Sun, Jul 22, 2007 at 07:52:36AM +0200, Jens Axboe wrote:
> > On Sun, Jul 22 2007, Andi Kleen wrote:
> > > Lars Ellenberg <lars@...bit.com> 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,

That's not needed for review. The final submission can then be
in a single patch. It's just impossible to read a really large
single patch. Ideally you space the posting also out over time.to not
tax the review resources too much.

Another standard trick for compileable patch series is to only add the 
Kconfig at the end.

> 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?

Preferable yes.

> 
>  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?

No, you can ignore it then.

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

Probably not.

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

Yes smaller functions are better in general.

-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/