Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1232703

Re: [PATCH v11 3/4] add FPGA manager core

Show all headers | View raw

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

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH v11 3/4] add FPGA manager core Josh Cartwright <joshc@ni.com> - 2015-09-23 00:40 +0200
  Re: [PATCH v11 3/4] add FPGA manager core Pavel Machek <pavel@denx.de> - 2015-09-23 15:30 +0200
    Re: [PATCH v11 3/4] add FPGA manager core Dan Carpenter <dan.carpenter@oracle.com> - 2015-09-23 16:20 +0200
      Re: [PATCH v11 3/4] add FPGA manager core Dan Carpenter <dan.carpenter@oracle.com> - 2015-09-24 10:00 +0200
        Re: [PATCH v11 3/4] add FPGA manager core Pavel Machek <pavel@denx.de> - 2015-09-24 23:20 +0200
        Re: [PATCH v11 3/4] add FPGA manager core Dan Carpenter <dan.carpenter@oracle.com> - 2015-09-25 12:10 +0200
  Re: [PATCH v11 3/4] add FPGA manager core Josh Cartwright <joshc@ni.com> - 2015-09-24 01:10 +0200

csiph-web