lkml.org 
[lkml]   [2015]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v11 3/4] add FPGA manager core
On Thu, Sep 24, 2015 at 03:47:26PM -0500, atull wrote:
> Interesting. The amount of code bloat here compiles down to about two
> machine instructions (in two places). Actually a little more since I should
> be using IS_ERR_OR_NULL. But the main question is whether I should do
> it at all.
>

They kernel already has too many bogus checks for IS_ERR(). It's a very
common bug to check for IS_ERR() when you should be checking for NULL.

- foo = some_allocator();
+ foo = kmalloc();
if (IS_ERR(foo))
goto fail;

I have a static checker for "warn: 'foo' isn't an ERR_PTR" but I haven't
published it because too much code has impossible checks.

> The behaviour I should drive here is that the user will do their own error
> checking. After they get a pointer to a FPGA manager using
> of_fpga_mgr_get(), they should check it and not assume that
> fpga_mgr_firmware_load() will do it for them, i.e.
>
> mgr = of_fpga_mgr_get(mgr_node);
> if (IS_ERR(mgr))
> return PTR_ERR(mgr);
> fpga_mgr_firmware_load(mgr, flags, path);
>

I don't understand completely how of_fpga_mgr_get() ever returns NULL.
A lot of the of_ functions return ERR_PTRs if OF_ is compiled in but
they return NULL if it's not. I think this is so people can build with
COMPILE_TEST so we get more coverage with static analysis?

> I could take out these NULL pointer checks and it won't hurt anything unless
> someone is just using the functions badly, in which case: kablooey.

Linux devs are very good about doing error checking. An early kablooey
is what we want for people who don't.

Also if you provide a sanity check then Markus Elfring will remove all
the error checking in the callers. Don't do it.

regards,
dan carpenter



\
 
 \ /
  Last update: 2015-09-25 12:21    [W:0.151 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site