[05/10] validation: packet: print reason for suite init failure

Message ID 1486384684-14761-6-git-send-email-petri.savolainen@linaro.org
State Superseded
Headers show
Series
  • Packet function inline
Related show

Commit Message

Petri Savolainen Feb. 6, 2017, 12:37 p.m.
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

Comments

Maxim Uvarov Feb. 6, 2017, 2:34 p.m. | #1
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", &params);

> -	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",

>  					       &params);

>  	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;

>
Savolainen, Petri (Nokia - FI/Espoo) Feb. 6, 2017, 2:40 p.m. | #2
> -----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
Mike Holmes Feb. 6, 2017, 2:40 p.m. | #3
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", &params);

>> -     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",

>>                                              &params);

>>       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"
Maxim Uvarov Feb. 6, 2017, 2:48 p.m. | #4
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.
Savolainen, Petri (Nokia - FI/Espoo) Feb. 6, 2017, 2:49 p.m. | #5
> -----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
Maxim Uvarov Feb. 6, 2017, 2:55 p.m. | #6
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.
Mike Holmes Feb. 6, 2017, 3:01 p.m. | #7
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"
Maxim Uvarov Feb. 6, 2017, 3:05 p.m. | #8
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.

>>

> 

> 

>
Savolainen, Petri (Nokia - FI/Espoo) Feb. 6, 2017, 3:11 p.m. | #9
> -----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 Feb. 6, 2017, 5:12 p.m. | #10
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"
Bill Fischofer Feb. 6, 2017, 8:53 p.m. | #11
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"

Patch

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", &params);
-	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",
 					       &params);
 	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;