[ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fixoff-by-one

Glenn Streiff gstreiff at NetEffect.com
Thu Feb 21 11:46:14 PST 2008


 
> Looking at the patches what you did seems OK.
> 
> 
> But regarding "review" I have a different criticism directed 
> at Roland:
> 
> This driver should really have gotten some review before 
> being included 
> in the kernel.
> 
> Even a simple checkpatch run finds more than > 250 stylistic errors
> (not code bugs but cases where the driver violates the standard code 
> formatting rules of kernel code).
> 
> And I'm not talking about the > 2000 checkpatch warnings that 
> are mostly 
> about too long lines (which should arguably also be fixed).
> 
> And many more issues that could have been foung during a review.
> E.g. when you look at 3/8 from this series the code
>         if (!cm_node)
>                 return -EINVAL;
>         new_send = kzalloc(sizeof(*new_send), GFP_ATOMIC);
>         if (!new_send)
>                 return -1;
> doesn't look good since the -1 should most likely better be something 
> like -ENOMEM (I haven't checked whether you can immediately change it 
> at this specific place).
> 
> And these are just comments from someone with zero knowledge about 
> InfiniBand, but I'd expect InfiniBand-specifig bugs might be found 
> before they hit users if an InfiniBand maintainer would review the 
> complete driver.
> 
> Note that this is not meant as a criticism against Glenn - it's 
> normal that submitted code contains bugs, but a code review 
> can help to 
> cope with this.
> 
> > Glenn
> 
> cu
> Adrian
> 

Hi, Adrian.

Yeah, I agree that the stylistic issues are annoying and I am actually
itching to get some of those simples things corrected.  Roland has
outlined several areas for improvement in the driver (style-wise
and substance-wise) and I'm working to address those.  
I'm learning the ropes here so I expect I'll get faster/better
at responding and fixing things like the coverity issues you flagged.
I need to pull these tools into my own release process so I'm catching
flaws on my side.  I want the driver to be worthy.

Regards,

Glenn


More information about the general mailing list