lkml.org 
[lkml]   [2009]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Tux3 report: Tux3 Git tree available
On Wed, 11 Mar 2009 09:25:37 -0700
Daniel Phillips <phillips@phunq.net> wrote:

> The full patch is 191KB. We could try patchbombing lkml with that, one
> patch per file, say.

One patch per file is OK.

Two considerations:


- It should be reviewable! People don't want to spend all their
review time scratching their heads wondering "wtf is this piece of
code supposed to do".

Thoughtfully commented data structures and functions are valuable
during the code-review stage. If you try to retrofit them later then
reviewers get justifiably grumpy about all the time they wasted
ineffectually trying to review the uncommented stuff.

- Send the code for review when it's ready for linux-next
integration. I don't think it's a good idea to have it reviewed,
then you all disappear and spend three months changing it and then
put it up for linux-next integration.

OTOH, there may well be large changes as a result of review, so
don't leave it too late and avoid the temptation to think of it as
"finished", because it won't be!


Drive-by review:

- please prefer to leave a blank line between end-of-locals and
start-of-code in each fucntion.

- C++ comments make checkpatch (and kernel developers) whine.

- The non-tux3-specific bitmap-handling functions in balloc.c
shouldn't exist, I suspect. Use core kernel helpers. If they don't
exist, add them.

- bytebits() should use hweight8()

- No new typedefs, please. That means block_t. If there is some
real need to make block_t a typedef (such as: its size varies
according to Kconfig options) then grumble, OK. But it should then
be called tux3_block_t.

- count_range() is an unsuitable identifier for a kernel-wide symbol.
Please review all global symbols in the fs, ensure that they are
prefixed with "tux3_". Or make them static, of course.

- It uses printf() and assert()? Kernel code uses printk() and
BUG_ON(). Confused.

- There's a lot of this:

int ended = 0, any = 0;
struct buffer_head *buffer = blockread(mapping(inode), block);
if (!buffer)
return -1;
unsigned bytes = blocksize - offset;
if (bytes > tail)
bytes = tail;
unsigned char *p = bufdata(buffer) + offset, *top = p + bytes;

Which will spew compilation warnings due to local variable
definitions coming after code.

Maybe this code is never to be compiled into the kernel image, but
we might as well be consistent.

- What's "L"?

printf("%Lx-", (L)begin);

- When 'bh' is known to be non-NULL, use put_bh() rather than brelse().

- Use __packed, not PACKED.

- Run `checkpatch --file', enjoy the result.

- get_buffer() looks like it's racy against truncate. Needs to lock
the page.

- eh?

typedef u64 fixed32; /* Tux3 time values */

- link_entry() looks dangerous.

- Move SB_MAGIC to magic.h, change name.

- remove private hexdump(), use lib/hexdump.c


Generally: the code is _very_ namespace-piggy. Lots and lots of very
generic-sounding identifiers.



\
 
 \ /
  Last update: 2009-03-11 19:47    [W:0.128 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site