[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Mike Frysinger]

On Monday, July 11, 2011 05:53:49 Helmut Raiger wrote:
> On 07/07/2011 07:46 PM, Mike Frysinger wrote:
> > those NULL checks should not be necessary either.  a correctly written
> > networking driver should only register itself with the miiphy layer
> > when it has successfully registered itself with the eth layer.  thus
> > any of the miiphy callbacks should always come in with a name that is
> > found via eth_get_dev_by_name().
> 
> You are right, in a perfect world nobody ever falters.

you missed the point.  either the driver works, or it doesnt.  this func is 
used in such a way that the behavior is fairly deterministic (as i clearly 
laid out).  it isnt relying on allocated memory that could fall to be 
allocated for example.

> > checking for NULLs here and gracefully returning is unnecessary
> > overhead imo as you're only catering to broken code.  fix the broken
> > drivers instead.
> 
> I could not find drivers that have the potential of delivering NULLs to
> the miiphy_read and write functions, I simply refused to test at  this
> level. Either there is a potential of having NULL passed then the test
> should be in eth_get_dev_by_name() or there is none then we should
> simply leave it away.

i did go through the level of detail and showed the call graphs ... none of 
which should allow a driver tested as working to even once hit the NULL path.

> I'm rather indifferent to either solution.
> And about 'catering to broken code': It must be broken in 2 ways, first
> it must pass a NULL to the function and second it must ignore the return
> code.

not necessarily.  many platforms will abort on NULL pointer accesses.

> > by your logic, why put the NULL check in eth_get_dev_by_name() ?  why
> > not handle strcmp(NULL, NULL) too ?  then eth_get_dev_by_name() would
> > automatically get "fixed" as would all other call points.
> 
> For strcmp() you have several issues at hand: Compatibility, performance
> and even a logical problem. What should be the result in case one of the
> arguments is NULL (or both).

fair enough on the API, but your performance aspect is kind of hard to swallow 
when you turn around and say that NULL pointer checking elsewhere is 
minuscule.

> And finally we are discussing a few _chararacters_ of code and a
> probably a single assembly instruction.

it's not a single assembly insn.  try generating it.  it adds like 8 to my 
platform, mostly because of the increased register pressure.

but the point isnt the impact of this single check.  it sets the precedence 
that every function in u-boot that takes a pointer should start over 
protecting itself against poorly written code originating elsewhere.  now your 
"few characters" is quite a bit more.

what i wouldnt mind is annotating the prototype with gcc attributes saying 
that the argument is nonnull.
...
#define __nonnull(x) __attribute__((__nonnull__ x))
...
extern struct eth_device *eth_get_dev_by_name(const char *devname) 
__nonnull(1);
...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110712/11806c4f/attachment.pgp