| Message ID | 1486384684-14761-6-git-send-email-petri.savolainen@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Packet function inline | expand |
On 02/06/17 15:37, Petri Savolainen wrote: > Knowing the reason for suite init function failure helps in > debugging. > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > test/common_plat/validation/api/packet/packet.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/test/common_plat/validation/api/packet/packet.c b/test/common_plat/validation/api/packet/packet.c > index fa5206f..e3d28f6 100644 > --- a/test/common_plat/validation/api/packet/packet.c > +++ b/test/common_plat/validation/api/packet/packet.c > @@ -110,8 +110,10 @@ int packet_suite_init(void) > uint8_t data = 0; > uint32_t i; > > - if (odp_pool_capability(&capa) < 0) > + if (odp_pool_capability(&capa) < 0) { > + printf("pool_capability failed\n"); > return -1; > + } it's it better to return -1, -2, -3 and put debug print in upper function? Here: /* execute its init function */ if (sinfo->pInitFunc) { ret = sinfo->pInitFunc(); if (ret) return ret; } or it can be CU_FAIL(msg) which already writes line number. Maxim. > > /* Pick a typical packet size and decrement it to the single segment > * limit if needed (min_seg_len maybe equal to max_len > @@ -136,14 +138,17 @@ int packet_suite_init(void) > params.pkt.uarea_size = sizeof(struct udata_struct); > > packet_pool = odp_pool_create("packet_pool", ¶ms); > - if (packet_pool == ODP_POOL_INVALID) > + if (packet_pool == ODP_POOL_INVALID) { > + printf("pool_create failed: 1\n"); > return -1; > + } > > params.pkt.uarea_size = 0; > packet_pool_no_uarea = odp_pool_create("packet_pool_no_uarea", > ¶ms); > if (packet_pool_no_uarea == ODP_POOL_INVALID) { > odp_pool_destroy(packet_pool); > + printf("pool_create failed: 2\n"); > return -1; > } > > @@ -154,6 +159,7 @@ int packet_suite_init(void) > if (packet_pool_double_uarea == ODP_POOL_INVALID) { > odp_pool_destroy(packet_pool_no_uarea); > odp_pool_destroy(packet_pool); > + printf("pool_create failed: 3\n"); > return -1; > } > > @@ -174,8 +180,10 @@ int packet_suite_init(void) > } while (segmented_test_packet == ODP_PACKET_INVALID); > > if (odp_packet_is_valid(test_packet) == 0 || > - odp_packet_is_valid(segmented_test_packet) == 0) > + odp_packet_is_valid(segmented_test_packet) == 0) { > + printf("packet_is_valid failed\n"); > return -1; > + } > > segmentation_supported = odp_packet_is_segmented(segmented_test_packet); > > @@ -187,16 +195,21 @@ int packet_suite_init(void) > > udat = odp_packet_user_area(test_packet); > udat_size = odp_packet_user_area_size(test_packet); > - if (!udat || udat_size != sizeof(struct udata_struct)) > + if (!udat || udat_size != sizeof(struct udata_struct)) { > + printf("packet_user_area failed: 1\n"); > return -1; > + } > > odp_pool_print(packet_pool); > memcpy(udat, &test_packet_udata, sizeof(struct udata_struct)); > > udat = odp_packet_user_area(segmented_test_packet); > udat_size = odp_packet_user_area_size(segmented_test_packet); > - if (udat == NULL || udat_size != sizeof(struct udata_struct)) > + if (udat == NULL || udat_size != sizeof(struct udata_struct)) { > + printf("packet_user_area failed: 2\n"); > return -1; > + } > + > memcpy(udat, &test_packet_udata, sizeof(struct udata_struct)); > > return 0; >
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Maxim > Uvarov > Sent: Monday, February 06, 2017 4:34 PM > To: lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for > suite init failure > > On 02/06/17 15:37, Petri Savolainen wrote: > > Knowing the reason for suite init function failure helps in > > debugging. > > > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > > --- > > test/common_plat/validation/api/packet/packet.c | 23 > ++++++++++++++++++----- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/test/common_plat/validation/api/packet/packet.c > b/test/common_plat/validation/api/packet/packet.c > > index fa5206f..e3d28f6 100644 > > --- a/test/common_plat/validation/api/packet/packet.c > > +++ b/test/common_plat/validation/api/packet/packet.c > > @@ -110,8 +110,10 @@ int packet_suite_init(void) > > uint8_t data = 0; > > uint32_t i; > > > > - if (odp_pool_capability(&capa) < 0) > > + if (odp_pool_capability(&capa) < 0) { > > + printf("pool_capability failed\n"); > > return -1; > > + } > > > it's it better to return -1, -2, -3 and put debug print in upper > function? Here: > > > /* execute its init function */ > if (sinfo->pInitFunc) { > ret = sinfo->pInitFunc(); > if (ret) > return ret; > } > > or it can be CU_FAIL(msg) which already writes line number. > > > Maxim. This is CUnit init time function, which cannot handle CU_xxx calls. At least, other validation init calls used printf directly. I think "pool_capability failed" is easier to (grep and) find than -2. -Petri
On 6 February 2017 at 09:34, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 02/06/17 15:37, Petri Savolainen wrote: >> Knowing the reason for suite init function failure helps in >> debugging. >> >> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >> --- >> test/common_plat/validation/api/packet/packet.c | 23 ++++++++++++++++++----- >> 1 file changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/test/common_plat/validation/api/packet/packet.c b/test/common_plat/validation/api/packet/packet.c >> index fa5206f..e3d28f6 100644 >> --- a/test/common_plat/validation/api/packet/packet.c >> +++ b/test/common_plat/validation/api/packet/packet.c >> @@ -110,8 +110,10 @@ int packet_suite_init(void) >> uint8_t data = 0; >> uint32_t i; >> >> - if (odp_pool_capability(&capa) < 0) >> + if (odp_pool_capability(&capa) < 0) { >> + printf("pool_capability failed\n"); We have defined LOG_DBG in test_debug.h, shoudl we be using that ? >> return -1; >> + } > > > it's it better to return -1, -2, -3 and put debug print in upper > function? Here: > > > /* execute its init function */ > if (sinfo->pInitFunc) { > ret = sinfo->pInitFunc(); > if (ret) > return ret; > } > > or it can be CU_FAIL(msg) which already writes line number. > > > Maxim. > >> >> /* Pick a typical packet size and decrement it to the single segment >> * limit if needed (min_seg_len maybe equal to max_len >> @@ -136,14 +138,17 @@ int packet_suite_init(void) >> params.pkt.uarea_size = sizeof(struct udata_struct); >> >> packet_pool = odp_pool_create("packet_pool", ¶ms); >> - if (packet_pool == ODP_POOL_INVALID) >> + if (packet_pool == ODP_POOL_INVALID) { >> + printf("pool_create failed: 1\n"); >> return -1; >> + } >> >> params.pkt.uarea_size = 0; >> packet_pool_no_uarea = odp_pool_create("packet_pool_no_uarea", >> ¶ms); >> if (packet_pool_no_uarea == ODP_POOL_INVALID) { >> odp_pool_destroy(packet_pool); >> + printf("pool_create failed: 2\n"); >> return -1; >> } >> >> @@ -154,6 +159,7 @@ int packet_suite_init(void) >> if (packet_pool_double_uarea == ODP_POOL_INVALID) { >> odp_pool_destroy(packet_pool_no_uarea); >> odp_pool_destroy(packet_pool); >> + printf("pool_create failed: 3\n"); >> return -1; >> } >> >> @@ -174,8 +180,10 @@ int packet_suite_init(void) >> } while (segmented_test_packet == ODP_PACKET_INVALID); >> >> if (odp_packet_is_valid(test_packet) == 0 || >> - odp_packet_is_valid(segmented_test_packet) == 0) >> + odp_packet_is_valid(segmented_test_packet) == 0) { >> + printf("packet_is_valid failed\n"); >> return -1; >> + } >> >> segmentation_supported = odp_packet_is_segmented(segmented_test_packet); >> >> @@ -187,16 +195,21 @@ int packet_suite_init(void) >> >> udat = odp_packet_user_area(test_packet); >> udat_size = odp_packet_user_area_size(test_packet); >> - if (!udat || udat_size != sizeof(struct udata_struct)) >> + if (!udat || udat_size != sizeof(struct udata_struct)) { >> + printf("packet_user_area failed: 1\n"); >> return -1; >> + } >> >> odp_pool_print(packet_pool); >> memcpy(udat, &test_packet_udata, sizeof(struct udata_struct)); >> >> udat = odp_packet_user_area(segmented_test_packet); >> udat_size = odp_packet_user_area_size(segmented_test_packet); >> - if (udat == NULL || udat_size != sizeof(struct udata_struct)) >> + if (udat == NULL || udat_size != sizeof(struct udata_struct)) { >> + printf("packet_user_area failed: 2\n"); >> return -1; >> + } >> + >> memcpy(udat, &test_packet_udata, sizeof(struct udata_struct)); >> >> return 0; >> > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org │ Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
On 02/06/17 17:40, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Maxim >> Uvarov >> Sent: Monday, February 06, 2017 4:34 PM >> To: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for >> suite init failure >> >> On 02/06/17 15:37, Petri Savolainen wrote: >>> Knowing the reason for suite init function failure helps in >>> debugging. >>> >>> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >>> --- >>> test/common_plat/validation/api/packet/packet.c | 23 >> ++++++++++++++++++----- >>> 1 file changed, 18 insertions(+), 5 deletions(-) >>> >>> diff --git a/test/common_plat/validation/api/packet/packet.c >> b/test/common_plat/validation/api/packet/packet.c >>> index fa5206f..e3d28f6 100644 >>> --- a/test/common_plat/validation/api/packet/packet.c >>> +++ b/test/common_plat/validation/api/packet/packet.c >>> @@ -110,8 +110,10 @@ int packet_suite_init(void) >>> uint8_t data = 0; >>> uint32_t i; >>> >>> - if (odp_pool_capability(&capa) < 0) >>> + if (odp_pool_capability(&capa) < 0) { >>> + printf("pool_capability failed\n"); >>> return -1; >>> + } >> >> >> it's it better to return -1, -2, -3 and put debug print in upper >> function? Here: >> >> >> /* execute its init function */ >> if (sinfo->pInitFunc) { >> ret = sinfo->pInitFunc(); >> if (ret) >> return ret; >> } >> >> or it can be CU_FAIL(msg) which already writes line number. >> >> >> Maxim. > > This is CUnit init time function, which cannot handle CU_xxx calls. At least, other validation init calls used printf directly. I think "pool_capability failed" is easier to (grep and) find than -2. > > -Petri > yes, init function does not support CU calls, forgot about that. Yes, grep should be more easy. Ok, with that patch. Maxim.
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Mike > Holmes > Sent: Monday, February 06, 2017 4:41 PM > To: Maxim Uvarov <maxim.uvarov@linaro.org> > Cc: lng-odp <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for > suite init failure > > On 6 February 2017 at 09:34, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > On 02/06/17 15:37, Petri Savolainen wrote: > >> Knowing the reason for suite init function failure helps in > >> debugging. > >> > >> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > >> --- > >> test/common_plat/validation/api/packet/packet.c | 23 > ++++++++++++++++++----- > >> 1 file changed, 18 insertions(+), 5 deletions(-) > >> > >> diff --git a/test/common_plat/validation/api/packet/packet.c > b/test/common_plat/validation/api/packet/packet.c > >> index fa5206f..e3d28f6 100644 > >> --- a/test/common_plat/validation/api/packet/packet.c > >> +++ b/test/common_plat/validation/api/packet/packet.c > >> @@ -110,8 +110,10 @@ int packet_suite_init(void) > >> uint8_t data = 0; > >> uint32_t i; > >> > >> - if (odp_pool_capability(&capa) < 0) > >> + if (odp_pool_capability(&capa) < 0) { > >> + printf("pool_capability failed\n"); > > We have defined LOG_DBG in test_debug.h, shoudl we be using that ? > All other xxx_suite_init() just use printf() or don't print at all. This is just applying the current practice. -Petri
On 02/06/17 17:49, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Mike >> Holmes >> Sent: Monday, February 06, 2017 4:41 PM >> To: Maxim Uvarov <maxim.uvarov@linaro.org> >> Cc: lng-odp <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for >> suite init failure >> >> On 6 February 2017 at 09:34, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>> On 02/06/17 15:37, Petri Savolainen wrote: >>>> Knowing the reason for suite init function failure helps in >>>> debugging. >>>> >>>> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >>>> --- >>>> test/common_plat/validation/api/packet/packet.c | 23 >> ++++++++++++++++++----- >>>> 1 file changed, 18 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/test/common_plat/validation/api/packet/packet.c >> b/test/common_plat/validation/api/packet/packet.c >>>> index fa5206f..e3d28f6 100644 >>>> --- a/test/common_plat/validation/api/packet/packet.c >>>> +++ b/test/common_plat/validation/api/packet/packet.c >>>> @@ -110,8 +110,10 @@ int packet_suite_init(void) >>>> uint8_t data = 0; >>>> uint32_t i; >>>> >>>> - if (odp_pool_capability(&capa) < 0) >>>> + if (odp_pool_capability(&capa) < 0) { >>>> + printf("pool_capability failed\n"); >> >> We have defined LOG_DBG in test_debug.h, shoudl we be using that ? >> > > All other xxx_suite_init() just use printf() or don't print at all. This is just applying the current practice. > > -Petri > > LOG_ for implementation only, not for tests. Maxim.
On 6 February 2017 at 09:55, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 02/06/17 17:49, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >> >>> -----Original Message----- >>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Mike >>> Holmes >>> Sent: Monday, February 06, 2017 4:41 PM >>> To: Maxim Uvarov <maxim.uvarov@linaro.org> >>> Cc: lng-odp <lng-odp@lists.linaro.org> >>> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for >>> suite init failure >>> >>> On 6 February 2017 at 09:34, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>>> On 02/06/17 15:37, Petri Savolainen wrote: >>>>> Knowing the reason for suite init function failure helps in >>>>> debugging. >>>>> >>>>> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >>>>> --- >>>>> test/common_plat/validation/api/packet/packet.c | 23 >>> ++++++++++++++++++----- >>>>> 1 file changed, 18 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/test/common_plat/validation/api/packet/packet.c >>> b/test/common_plat/validation/api/packet/packet.c >>>>> index fa5206f..e3d28f6 100644 >>>>> --- a/test/common_plat/validation/api/packet/packet.c >>>>> +++ b/test/common_plat/validation/api/packet/packet.c >>>>> @@ -110,8 +110,10 @@ int packet_suite_init(void) >>>>> uint8_t data = 0; >>>>> uint32_t i; >>>>> >>>>> - if (odp_pool_capability(&capa) < 0) >>>>> + if (odp_pool_capability(&capa) < 0) { >>>>> + printf("pool_capability failed\n"); >>> >>> We have defined LOG_DBG in test_debug.h, shoudl we be using that ? >>> >> >> All other xxx_suite_init() just use printf() or don't print at all. This is just applying the current practice. >> >> -Petri >> >> > > LOG_ for implementation only, not for tests. That is not true currently, but happy if we delete the current cases or document why we pick either method. common_plat/validation/api/system/system.c: LOG_DBG("\nBAD VERSION=%s\n", version_string); common_plat/validation/api/timer/timer.c: LOG_DBG("Timer handle: %" PRIu64 "\n", odp_timer_to_u64(tim)); common_plat/validation/api/timer/timer.c: LOG_DBG("Timeout handle: %" PRIu64 "\n", odp_timeout_to_u64(tmo)); common_plat/validation/api/timer/timer.c: LOG_DBG("Wrong tick: expected %" PRIu64 common_plat/validation/api/timer/timer.c: LOG_DBG("Too late tick: %" PRIu64 common_plat/validation/api/timer/timer.c: LOG_DBG("Failed to allocate timeout (%" PRIu32 "/%d)\n", common_plat/validation/api/timer/timer.c: LOG_DBG("Failed to allocate timer .... I think we should be consistent, looks like there are two standards, some with printf > > > Maxim. > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org │ Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
On 02/06/17 18:01, Mike Holmes wrote: > On 6 February 2017 at 09:55, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> On 02/06/17 17:49, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> >>> >>>> -----Original Message----- >>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Mike >>>> Holmes >>>> Sent: Monday, February 06, 2017 4:41 PM >>>> To: Maxim Uvarov <maxim.uvarov@linaro.org> >>>> Cc: lng-odp <lng-odp@lists.linaro.org> >>>> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for >>>> suite init failure >>>> >>>> On 6 February 2017 at 09:34, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>>>> On 02/06/17 15:37, Petri Savolainen wrote: >>>>>> Knowing the reason for suite init function failure helps in >>>>>> debugging. >>>>>> >>>>>> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >>>>>> --- >>>>>> test/common_plat/validation/api/packet/packet.c | 23 >>>> ++++++++++++++++++----- >>>>>> 1 file changed, 18 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/test/common_plat/validation/api/packet/packet.c >>>> b/test/common_plat/validation/api/packet/packet.c >>>>>> index fa5206f..e3d28f6 100644 >>>>>> --- a/test/common_plat/validation/api/packet/packet.c >>>>>> +++ b/test/common_plat/validation/api/packet/packet.c >>>>>> @@ -110,8 +110,10 @@ int packet_suite_init(void) >>>>>> uint8_t data = 0; >>>>>> uint32_t i; >>>>>> >>>>>> - if (odp_pool_capability(&capa) < 0) >>>>>> + if (odp_pool_capability(&capa) < 0) { >>>>>> + printf("pool_capability failed\n"); >>>> >>>> We have defined LOG_DBG in test_debug.h, shoudl we be using that ? >>>> >>> >>> All other xxx_suite_init() just use printf() or don't print at all. This is just applying the current practice. >>> >>> -Petri >>> >>> >> >> LOG_ for implementation only, not for tests. > > That is not true currently, but happy if we delete the current cases > or document why we pick either method. > > common_plat/validation/api/system/system.c: > LOG_DBG("\nBAD VERSION=%s\n", version_string); > common_plat/validation/api/timer/timer.c: LOG_DBG("Timer handle: > %" PRIu64 "\n", odp_timer_to_u64(tim)); > common_plat/validation/api/timer/timer.c: LOG_DBG("Timeout > handle: %" PRIu64 "\n", odp_timeout_to_u64(tmo)); > common_plat/validation/api/timer/timer.c: > LOG_DBG("Wrong tick: expected %" PRIu64 > common_plat/validation/api/timer/timer.c: > LOG_DBG("Too late tick: %" PRIu64 > common_plat/validation/api/timer/timer.c: > LOG_DBG("Failed to allocate timeout (%" PRIu32 "/%d)\n", > common_plat/validation/api/timer/timer.c: > LOG_DBG("Failed to allocate timer > .... > > > I think we should be consistent, looks like there are two standards, > some with printf unbelievable LOG_ are defined in test/test_debug.h In that case we should use them in tests. Maxim. > >> >> >> Maxim. >> > > >
> -----Original Message----- > From: Maxim Uvarov [mailto:maxim.uvarov@linaro.org] > Sent: Monday, February 06, 2017 5:05 PM > To: Mike Holmes <mike.holmes@linaro.org> > Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- > labs.com>; lng-odp <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for > suite init failure > > On 02/06/17 18:01, Mike Holmes wrote: > > On 6 February 2017 at 09:55, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > >> On 02/06/17 17:49, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Mike > >>>> Holmes > >>>> Sent: Monday, February 06, 2017 4:41 PM > >>>> To: Maxim Uvarov <maxim.uvarov@linaro.org> > >>>> Cc: lng-odp <lng-odp@lists.linaro.org> > >>>> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason > for > >>>> suite init failure > >>>> > >>>> On 6 February 2017 at 09:34, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > >>>>> On 02/06/17 15:37, Petri Savolainen wrote: > >>>>>> Knowing the reason for suite init function failure helps in > >>>>>> debugging. > >>>>>> > >>>>>> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > >>>>>> --- > >>>>>> test/common_plat/validation/api/packet/packet.c | 23 > >>>> ++++++++++++++++++----- > >>>>>> 1 file changed, 18 insertions(+), 5 deletions(-) > >>>>>> > >>>>>> diff --git a/test/common_plat/validation/api/packet/packet.c > >>>> b/test/common_plat/validation/api/packet/packet.c > >>>>>> index fa5206f..e3d28f6 100644 > >>>>>> --- a/test/common_plat/validation/api/packet/packet.c > >>>>>> +++ b/test/common_plat/validation/api/packet/packet.c > >>>>>> @@ -110,8 +110,10 @@ int packet_suite_init(void) > >>>>>> uint8_t data = 0; > >>>>>> uint32_t i; > >>>>>> > >>>>>> - if (odp_pool_capability(&capa) < 0) > >>>>>> + if (odp_pool_capability(&capa) < 0) { > >>>>>> + printf("pool_capability failed\n"); > >>>> > >>>> We have defined LOG_DBG in test_debug.h, shoudl we be using that ? > >>>> > >>> > >>> All other xxx_suite_init() just use printf() or don't print at all. > This is just applying the current practice. > >>> > >>> -Petri > >>> > >>> > >> > >> LOG_ for implementation only, not for tests. > > > > That is not true currently, but happy if we delete the current cases > > or document why we pick either method. > > > > common_plat/validation/api/system/system.c: > > LOG_DBG("\nBAD VERSION=%s\n", version_string); > > common_plat/validation/api/timer/timer.c: LOG_DBG("Timer handle: > > %" PRIu64 "\n", odp_timer_to_u64(tim)); > > common_plat/validation/api/timer/timer.c: LOG_DBG("Timeout > > handle: %" PRIu64 "\n", odp_timeout_to_u64(tmo)); > > common_plat/validation/api/timer/timer.c: > > LOG_DBG("Wrong tick: expected %" PRIu64 > > common_plat/validation/api/timer/timer.c: > > LOG_DBG("Too late tick: %" PRIu64 > > common_plat/validation/api/timer/timer.c: > > LOG_DBG("Failed to allocate timeout (%" PRIu32 "/%d)\n", > > common_plat/validation/api/timer/timer.c: > > LOG_DBG("Failed to allocate timer > > .... > > > > > > I think we should be consistent, looks like there are two standards, > > some with printf > > > unbelievable LOG_ are defined in test/test_debug.h > In that case we should use them in tests. > > Maxim. Found over 200 hits of printf() in current validation test .c files. May be someone can take an action to clean out all of those. This patch just follows the current practice of xxx_suite_init() functions. -Petri
Call for objections to removing the macro ? Sounds reasonable to me, and I dont think we need to block this going in, cleaning up is a seperate issue so I will make a bug for it On 6 February 2017 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: > > >> -----Original Message----- >> From: Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >> Sent: Monday, February 06, 2017 5:05 PM >> To: Mike Holmes <mike.holmes@linaro.org> >> Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- >> labs.com>; lng-odp <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for >> suite init failure >> >> On 02/06/17 18:01, Mike Holmes wrote: >> > On 6 February 2017 at 09:55, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >> >> On 02/06/17 17:49, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >>> >> >>> >> >>>> -----Original Message----- >> >>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> Mike >> >>>> Holmes >> >>>> Sent: Monday, February 06, 2017 4:41 PM >> >>>> To: Maxim Uvarov <maxim.uvarov@linaro.org> >> >>>> Cc: lng-odp <lng-odp@lists.linaro.org> >> >>>> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason >> for >> >>>> suite init failure >> >>>> >> >>>> On 6 February 2017 at 09:34, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >> >>>>> On 02/06/17 15:37, Petri Savolainen wrote: >> >>>>>> Knowing the reason for suite init function failure helps in >> >>>>>> debugging. >> >>>>>> >> >>>>>> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >> >>>>>> --- >> >>>>>> test/common_plat/validation/api/packet/packet.c | 23 >> >>>> ++++++++++++++++++----- >> >>>>>> 1 file changed, 18 insertions(+), 5 deletions(-) >> >>>>>> >> >>>>>> diff --git a/test/common_plat/validation/api/packet/packet.c >> >>>> b/test/common_plat/validation/api/packet/packet.c >> >>>>>> index fa5206f..e3d28f6 100644 >> >>>>>> --- a/test/common_plat/validation/api/packet/packet.c >> >>>>>> +++ b/test/common_plat/validation/api/packet/packet.c >> >>>>>> @@ -110,8 +110,10 @@ int packet_suite_init(void) >> >>>>>> uint8_t data = 0; >> >>>>>> uint32_t i; >> >>>>>> >> >>>>>> - if (odp_pool_capability(&capa) < 0) >> >>>>>> + if (odp_pool_capability(&capa) < 0) { >> >>>>>> + printf("pool_capability failed\n"); >> >>>> >> >>>> We have defined LOG_DBG in test_debug.h, shoudl we be using that ? >> >>>> >> >>> >> >>> All other xxx_suite_init() just use printf() or don't print at all. >> This is just applying the current practice. >> >>> >> >>> -Petri >> >>> >> >>> >> >> >> >> LOG_ for implementation only, not for tests. >> > >> > That is not true currently, but happy if we delete the current cases >> > or document why we pick either method. >> > >> > common_plat/validation/api/system/system.c: >> > LOG_DBG("\nBAD VERSION=%s\n", version_string); >> > common_plat/validation/api/timer/timer.c: LOG_DBG("Timer handle: >> > %" PRIu64 "\n", odp_timer_to_u64(tim)); >> > common_plat/validation/api/timer/timer.c: LOG_DBG("Timeout >> > handle: %" PRIu64 "\n", odp_timeout_to_u64(tmo)); >> > common_plat/validation/api/timer/timer.c: >> > LOG_DBG("Wrong tick: expected %" PRIu64 >> > common_plat/validation/api/timer/timer.c: >> > LOG_DBG("Too late tick: %" PRIu64 >> > common_plat/validation/api/timer/timer.c: >> > LOG_DBG("Failed to allocate timeout (%" PRIu32 "/%d)\n", >> > common_plat/validation/api/timer/timer.c: >> > LOG_DBG("Failed to allocate timer >> > .... >> > >> > >> > I think we should be consistent, looks like there are two standards, >> > some with printf >> >> >> unbelievable LOG_ are defined in test/test_debug.h >> In that case we should use them in tests. >> >> Maxim. > > Found over 200 hits of printf() in current validation test .c files. May be someone can take an action to clean out all of those. This patch just follows the current practice of xxx_suite_init() functions. > > -Petri > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org │ Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
I'm all for keeping things simple and just using printf() or other standard C library functions in validation tests. The purpose of the macros in ODP code was to allow for transparent redirection of log messages to an application-supplied logger, and that's not something that we need for the validation tests. The same could be said for the similar macros used in the examples, but I see no urgency to make changes there. On Mon, Feb 6, 2017 at 11:12 AM, Mike Holmes <mike.holmes@linaro.org> wrote: > Call for objections to removing the macro ? > > Sounds reasonable to me, and I dont think we need to block this going > in, cleaning up is a seperate issue so I will make a bug for it > > On 6 February 2017 at 10:11, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia-bell-labs.com> wrote: >> >> >>> -----Original Message----- >>> From: Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >>> Sent: Monday, February 06, 2017 5:05 PM >>> To: Mike Holmes <mike.holmes@linaro.org> >>> Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- >>> labs.com>; lng-odp <lng-odp@lists.linaro.org> >>> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason for >>> suite init failure >>> >>> On 02/06/17 18:01, Mike Holmes wrote: >>> > On 6 February 2017 at 09:55, Maxim Uvarov <maxim.uvarov@linaro.org> >>> wrote: >>> >> On 02/06/17 17:49, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> >>> >>> >>> >>> >>>> -----Original Message----- >>> >>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >>> Mike >>> >>>> Holmes >>> >>>> Sent: Monday, February 06, 2017 4:41 PM >>> >>>> To: Maxim Uvarov <maxim.uvarov@linaro.org> >>> >>>> Cc: lng-odp <lng-odp@lists.linaro.org> >>> >>>> Subject: Re: [lng-odp] [PATCH 05/10] validation: packet: print reason >>> for >>> >>>> suite init failure >>> >>>> >>> >>>> On 6 February 2017 at 09:34, Maxim Uvarov <maxim.uvarov@linaro.org> >>> wrote: >>> >>>>> On 02/06/17 15:37, Petri Savolainen wrote: >>> >>>>>> Knowing the reason for suite init function failure helps in >>> >>>>>> debugging. >>> >>>>>> >>> >>>>>> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >>> >>>>>> --- >>> >>>>>> test/common_plat/validation/api/packet/packet.c | 23 >>> >>>> ++++++++++++++++++----- >>> >>>>>> 1 file changed, 18 insertions(+), 5 deletions(-) >>> >>>>>> >>> >>>>>> diff --git a/test/common_plat/validation/api/packet/packet.c >>> >>>> b/test/common_plat/validation/api/packet/packet.c >>> >>>>>> index fa5206f..e3d28f6 100644 >>> >>>>>> --- a/test/common_plat/validation/api/packet/packet.c >>> >>>>>> +++ b/test/common_plat/validation/api/packet/packet.c >>> >>>>>> @@ -110,8 +110,10 @@ int packet_suite_init(void) >>> >>>>>> uint8_t data = 0; >>> >>>>>> uint32_t i; >>> >>>>>> >>> >>>>>> - if (odp_pool_capability(&capa) < 0) >>> >>>>>> + if (odp_pool_capability(&capa) < 0) { >>> >>>>>> + printf("pool_capability failed\n"); >>> >>>> >>> >>>> We have defined LOG_DBG in test_debug.h, shoudl we be using that ? >>> >>>> >>> >>> >>> >>> All other xxx_suite_init() just use printf() or don't print at all. >>> This is just applying the current practice. >>> >>> >>> >>> -Petri >>> >>> >>> >>> >>> >> >>> >> LOG_ for implementation only, not for tests. >>> > >>> > That is not true currently, but happy if we delete the current cases >>> > or document why we pick either method. >>> > >>> > common_plat/validation/api/system/system.c: >>> > LOG_DBG("\nBAD VERSION=%s\n", version_string); >>> > common_plat/validation/api/timer/timer.c: LOG_DBG("Timer handle: >>> > %" PRIu64 "\n", odp_timer_to_u64(tim)); >>> > common_plat/validation/api/timer/timer.c: LOG_DBG("Timeout >>> > handle: %" PRIu64 "\n", odp_timeout_to_u64(tmo)); >>> > common_plat/validation/api/timer/timer.c: >>> > LOG_DBG("Wrong tick: expected %" PRIu64 >>> > common_plat/validation/api/timer/timer.c: >>> > LOG_DBG("Too late tick: %" PRIu64 >>> > common_plat/validation/api/timer/timer.c: >>> > LOG_DBG("Failed to allocate timeout (%" PRIu32 "/%d)\n", >>> > common_plat/validation/api/timer/timer.c: >>> > LOG_DBG("Failed to allocate timer >>> > .... >>> > >>> > >>> > I think we should be consistent, looks like there are two standards, >>> > some with printf >>> >>> >>> unbelievable LOG_ are defined in test/test_debug.h >>> In that case we should use them in tests. >>> >>> Maxim. >> >> Found over 200 hits of printf() in current validation test .c files. May be someone can take an action to clean out all of those. This patch just follows the current practice of xxx_suite_init() functions. >> >> -Petri >> > > > > -- > Mike Holmes > Program Manager - Linaro Networking Group > Linaro.org │ Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows"
diff --git a/test/common_plat/validation/api/packet/packet.c b/test/common_plat/validation/api/packet/packet.c index fa5206f..e3d28f6 100644 --- a/test/common_plat/validation/api/packet/packet.c +++ b/test/common_plat/validation/api/packet/packet.c @@ -110,8 +110,10 @@ int packet_suite_init(void) uint8_t data = 0; uint32_t i; - if (odp_pool_capability(&capa) < 0) + if (odp_pool_capability(&capa) < 0) { + printf("pool_capability failed\n"); return -1; + } /* Pick a typical packet size and decrement it to the single segment * limit if needed (min_seg_len maybe equal to max_len @@ -136,14 +138,17 @@ int packet_suite_init(void) params.pkt.uarea_size = sizeof(struct udata_struct); packet_pool = odp_pool_create("packet_pool", ¶ms); - if (packet_pool == ODP_POOL_INVALID) + if (packet_pool == ODP_POOL_INVALID) { + printf("pool_create failed: 1\n"); return -1; + } params.pkt.uarea_size = 0; packet_pool_no_uarea = odp_pool_create("packet_pool_no_uarea", ¶ms); if (packet_pool_no_uarea == ODP_POOL_INVALID) { odp_pool_destroy(packet_pool); + printf("pool_create failed: 2\n"); return -1; } @@ -154,6 +159,7 @@ int packet_suite_init(void) if (packet_pool_double_uarea == ODP_POOL_INVALID) { odp_pool_destroy(packet_pool_no_uarea); odp_pool_destroy(packet_pool); + printf("pool_create failed: 3\n"); return -1; } @@ -174,8 +180,10 @@ int packet_suite_init(void) } while (segmented_test_packet == ODP_PACKET_INVALID); if (odp_packet_is_valid(test_packet) == 0 || - odp_packet_is_valid(segmented_test_packet) == 0) + odp_packet_is_valid(segmented_test_packet) == 0) { + printf("packet_is_valid failed\n"); return -1; + } segmentation_supported = odp_packet_is_segmented(segmented_test_packet); @@ -187,16 +195,21 @@ int packet_suite_init(void) udat = odp_packet_user_area(test_packet); udat_size = odp_packet_user_area_size(test_packet); - if (!udat || udat_size != sizeof(struct udata_struct)) + if (!udat || udat_size != sizeof(struct udata_struct)) { + printf("packet_user_area failed: 1\n"); return -1; + } odp_pool_print(packet_pool); memcpy(udat, &test_packet_udata, sizeof(struct udata_struct)); udat = odp_packet_user_area(segmented_test_packet); udat_size = odp_packet_user_area_size(segmented_test_packet); - if (udat == NULL || udat_size != sizeof(struct udata_struct)) + if (udat == NULL || udat_size != sizeof(struct udata_struct)) { + printf("packet_user_area failed: 2\n"); return -1; + } + memcpy(udat, &test_packet_udata, sizeof(struct udata_struct)); return 0;
Knowing the reason for suite init function failure helps in debugging. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- test/common_plat/validation/api/packet/packet.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) -- 2.8.1