Re: [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>]

On Sun, Sep 28, 2014 at 05:38:06AM +0000, Wiles, Roger Keith wrote:
> 
> On Sep 27, 2014, at 8:55 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> 
> > On Sun, Sep 28, 2014 at 01:14:05AM +0000, Wiles, Roger Keith wrote:
> >> 
> >> On Sep 27, 2014, at 7:37 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> >> 
> >>> On Sat, Sep 27, 2014 at 06:35:01PM +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-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> >>>> ---
> >>>> lib/librte_mempool/rte_mempool.c | 6 ++++++
> >>>> 1 file changed, 6 insertions(+)
> >>>> 
> >>>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> >>>> index 332f469..efa6a6c 100644
> >>>> --- a/lib/librte_mempool/rte_mempool.c
> >>>> +++ b/lib/librte_mempool/rte_mempool.c
> >>>> @@ -765,6 +765,12 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
> >>>>   unsigned common_count;
> >>>>   unsigned cache_count;
> >>>> 
> >>>> +   if ( (f == NULL) || (mp == NULL) ) {
> >>>> +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> >>>> +       fprintf(stderr, "*** Called rte_mempool_dump(%p, %p) with NULL argument\n", f, mp);
> >>>> +#endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
> >>>> +       return;
> >>>> +   }
> >>>>   fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
> >>>>   fprintf(f, "  flags=%x\n", mp->flags);
> >>>>   fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
> >>>> -- 
> >>>> 2.1.0
> >>>> 
> >>>> 
> >>> Maybe use RTE_VERIFY instead?
> >>> Neil
> >>> 
> >> I did not think it needs to panic as it is just a debug function and returning would be fine by me, comments?
> >> Do we have a similar RTE_VERIFY like function that does not panic?
> >> 
> > If we don't, it would seem useful to make one.  It beats having to do specific
> > condition checking/error reporting.  RTE_VERIFY_WARN or some such.
> > Neil
> 
> I decided to just use RTE_VERIFY() instead of creating a new macro for now, it seems this maybe an isolated case. I agree having RTE_VERIFY_WARN() would be nice, but as I was writing the macro I wanted to return from the function. For this routine ‘return’ would work as it returns (void), but for other routines a value may need to be returned.
> 
Thats fine, you can do exactly what you need to do, just write the macro to
assert !!condition at the end, like this:
#define RTE_VERIFY_WARN(condition) do { \
    int ret = !!condition; \
    if (ret) \
        printf(<message>); \
    ret;\
}

Then, you can use the macro as a conditional itself anywhere you want:

int function(void *arguments)
{
    if (RTE_VERIFY(arguments == NULL))
        return 1
....
}

> Need a clean way to exit the routine without causing the macro to understand its return values. Just seem to become a bit messy at this point. Multiple macros for different return types or make the macros return a boolean value to be tested seemed to more complex then needed.
See above, thats how all the Linux WARN_ON macros work.

Neil

> > 
> >> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
>