[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