Re: [dpdk-dev] [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2014-10-01 17:05, Bruce Richardson:
> On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> > On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > > 2014-09-28 08:27, Neil Horman:
> > > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > > 
> > > > > > > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > > > > > 
> > > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > > macro here instead using what I suggested in my other note
> > > > > 
> > > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > > such check on input parameters.
> > > > > A similar discussion already happened few months ago:
> > > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > > 
> > > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > > with a NULL check at all and just accept the crash as it is?
> > > >
> > > 
> > > In the general case:
> > > * Code in the datapath should not have things like NULL checks
> > > * However, datapath code should generally have a debug option which turns 
> > >   these checks on to help debugging if needed. 
> > > * Code not in the datapath probably should have these checks.
> > > 
> > Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> > datapath, its rather by definition a debug function, isn't it?
> > Neil
> 
> Yes, agreed.  [So it probably should have the NULL check].

I have many arguments to not do this check:
1) If it was a coding rule to do this kind of check, it should be done in
almost every functions.
2) It's quite common to not do this check, e.g. what happen with memcpy(NULL,NULL)?
3) Why check only NULL value? 1 and 2 are also some invalid values...

-- 
Thomas