| Message ID | DAE811FA-C1F1-444D-980C-311BCCE7FF3C@windriver.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 071D7683E; Sat, 27 Sep 2014 20:28:40 +0200 (CEST) Received: from mail1.windriver.com (mail1.windriver.com [147.11.146.13]) by dpdk.org (Postfix) with ESMTP id 165AC678C for <dev@dpdk.org>; Sat, 27 Sep 2014 20:28:38 +0200 (CEST) Received: from ALA-HCB.corp.ad.wrs.com (ala-hcb.corp.ad.wrs.com [147.11.189.41]) by mail1.windriver.com (8.14.9/8.14.5) with ESMTP id s8RIZ5XO017987 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=FAIL) for <dev@dpdk.org>; Sat, 27 Sep 2014 11:35:05 -0700 (PDT) Received: from ALA-MBB.corp.ad.wrs.com ([169.254.1.18]) by ALA-HCB.corp.ad.wrs.com ([147.11.189.41]) with mapi id 14.03.0174.001; Sat, 27 Sep 2014 11:35:02 -0700 From: "Wiles, Roger Keith" <keith.wiles@windriver.com> To: "<dev@dpdk.org>" <dev@dpdk.org> Thread-Topic: [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer. Thread-Index: AQHP2oG/7+zUgDJSJ0eBA/ewM6tenA== Date: Sat, 27 Sep 2014 18:35:01 +0000 Message-ID: <DAE811FA-C1F1-444D-980C-311BCCE7FF3C@windriver.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.25.40.166] Content-Type: text/plain; charset="us-ascii" Content-ID: <9F346B39A9E4C44F8625649EA55EE177@local> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: [dpdk-dev] [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Wiles, Roger Keith
Sept. 27, 2014, 6:35 p.m. UTC
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>
---
lib/librte_mempool/rte_mempool.c | 6 ++++++
1 file changed, 6 insertions(+)
Comments
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@windriver.com> > --- > 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
OK, let me look at RTE_VERIFY then and will send a new patch. On Sep 27, 2014, at 7:37 PM, Neil Horman <nhorman@tuxdriver.com> 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@windriver.com> >> --- >> 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 > Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
On Sep 27, 2014, at 7:37 PM, Neil Horman <nhorman@tuxdriver.com> 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@windriver.com> >> --- >> 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? Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
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@tuxdriver.com> 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@windriver.com> > >> --- > >> 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 > Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533 > >
On Sep 27, 2014, at 8:55 PM, Neil Horman <nhorman@tuxdriver.com> 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@tuxdriver.com> 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@windriver.com> >>>> --- >>>> 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. 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. > >> 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
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@tuxdriver.com> 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@tuxdriver.com> 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@windriver.com> > >>>> --- > >>>> 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 > >
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);