Re: [patch -next v2] mv643xx_eth: potential null dereference

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Jul 25, 2010 at 04:47:17PM +0200, walter harms wrote:

> >> IMHO it would be better to make sure that pd->t_clk,pd->tx_csum_limit
> >> have usefull values than adding a check but this is up to the maintainer.
> > 
> > I don't see the point of that at all.  We check against zero to see
> > whether the caller bothered to fill in the field at all, but if the
> > caller wants to pass in bogus values, that's up to the caller.
> 
> at first i have to admit i looked only at the patch.
> for me the situation looks this way:
> 
> You check the values for 0 (and uses default) or take what ever in pd is.
> The current code is setup like:
> 
>   1. check if pd is set
>   2. check if pd->value is non zero and use it
> 
> the whole "check X" can be avoided if you could create a pd with all values
> set to default and just take what comes from the user.

Why would you want to avoid "check X"?


> my objection agains this kind of code is that it is not obvious
> what some one is trying to archive
> (pd != NULL && pd->t_clk != 0) ? pd->t_clk : 133000000;
> 
> the pd check means: do i have a configuration  ?

Yep.


> the pd->t_clk != 0 means: should i use then or default ?

Yep.


> This is mixing two very different questions.

And?

You're arguing against the use of 'if (a && b)' in general?


> therefore my idea in the last posting to have a default init if (!pd)
> and then use the else to make clear that additional checks for
> pd->value are expected.

Again, I don't see the point of this, and I think we're just splitting
hairs here and wasting everyone's time.


> this this is the init code readability and simplicity should be king.

'if (foo != NULL && foo->value <op> <value>)' is a fairly common C
construct, in my opinion, and I don't see how expanding it into a
multi-line nested if construct will make it either more readable or
simpler.


> hope that helps,

Not really.


thanks,
Lennert
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux