[dpdk-dev] [PATCH] test/hash: solve unit test hash compilation error

[dpdk-dev] [PATCH] test/hash: solve unit test hash compilation error

[Dharmik Thakkar]

Enable print_key_info() function compilation always.

Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 test/test/test_hash.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/test/test/test_hash.c b/test/test/test_hash.c
index b3db9fd..13239e1 100644
--- a/test/test/test_hash.c
+++ b/test/test/test_hash.c
@@ -80,29 +80,23 @@ static uint32_t pseudo_hash(__attribute__((unused)) const void *keys,
 	return 3;
 }
 
+#define UNIT_TEST_HASH_VERBOSE	0
 /*
  * Print out result of unit test hash operation.
  */
-#if defined(UNIT_TEST_HASH_VERBOSE)
 static void print_key_info(const char *msg, const struct flow_key *key,
 								int32_t pos)
 {
-	uint8_t *p = (uint8_t *)key;
-	unsigned i;
-
-	printf("%s key:0x", msg);
-	for (i = 0; i < sizeof(struct flow_key); i++) {
-		printf("%02X", p[i]);
+	if (UNIT_TEST_HASH_VERBOSE) {
+		const uint8_t *p = (const uint8_t *)key;
+		unsigned i;
+
+		printf("%s key:0x", msg);
+		for (i = 0; i < sizeof(struct flow_key); i++)
+			printf("%02X", p[i]);
+		printf(" @ pos %d\n", pos);
 	}
-	printf(" @ pos %d\n", pos);
-}
-#else
-static void print_key_info(__attribute__((unused)) const char *msg,
-		__attribute__((unused)) const struct flow_key *key,
-		__attribute__((unused)) int32_t pos)
-{
 }
-#endif
 
 /* Keys used by unit test functions */
 static struct flow_key keys[5] = { {
-- 
2.7.4

Re: [dpdk-dev] [PATCH] test/hash: solve unit test hash compilation error

[Gavin Hu]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Dharmik Thakkar
> Sent: Monday, August 27, 2018 10:26 PM
> To: Bruce Richardson <bruce.richardson@intel.com>; Pablo de Lara
> <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Dharmik Thakkar
> <Dharmik.Thakkar@arm.com>
> Subject: [dpdk-dev] [PATCH] test/hash: solve unit test hash compilation error
>
> Enable print_key_info() function compilation always.
>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Acked-by: Gavin Hu <gavin.hu@arm.com>
> ---
>  test/test/test_hash.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/test/test/test_hash.c b/test/test/test_hash.c index
> b3db9fd..13239e1 100644
> --- a/test/test/test_hash.c
> +++ b/test/test/test_hash.c
> @@ -80,29 +80,23 @@ static uint32_t pseudo_hash(__attribute__((unused))
> const void *keys,
>  return 3;
>  }
>
> +#define UNIT_TEST_HASH_VERBOSE0
>  /*
>   * Print out result of unit test hash operation.
>   */
> -#if defined(UNIT_TEST_HASH_VERBOSE)
>  static void print_key_info(const char *msg, const struct flow_key *key,
>  int32_t pos)
>  {
> -uint8_t *p = (uint8_t *)key;
> -unsigned i;
> -
> -printf("%s key:0x", msg);
> -for (i = 0; i < sizeof(struct flow_key); i++) {
> -printf("%02X", p[i]);
> +if (UNIT_TEST_HASH_VERBOSE) {
> +const uint8_t *p = (const uint8_t *)key;
> +unsigned i;
> +
> +printf("%s key:0x", msg);
> +for (i = 0; i < sizeof(struct flow_key); i++)
> +printf("%02X", p[i]);
> +printf(" @ pos %d\n", pos);
>  }
> -printf(" @ pos %d\n", pos);
> -}
> -#else
> -static void print_key_info(__attribute__((unused)) const char *msg,
> -__attribute__((unused)) const struct flow_key *key,
> -__attribute__((unused)) int32_t pos)
> -{
>  }
> -#endif
>
>  /* Keys used by unit test functions */
>  static struct flow_key keys[5] = { {
> --
> 2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] [PATCH] test/hash: solve unit test hash compilation error

[Thomas Monjalon]

Hi,

27/08/2018 16:26, Dharmik Thakkar:
> Enable print_key_info() function compilation always.
> 
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

Please describe the issue you are fixing and add a tag "Fixes:"
to help knowing where it should be backported.

Thanks

[dpdk-dev] [PATCH v2] test/hash: solve unit test hash compilation error

[Dharmik Thakkar]

Enable print_key_info() function compilation always.

Fixes: af75078fece36 ("first public release")
Cc: stable@dpdk.org

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
v2:
* Fix checkpatch coding style issue
* Add "Fixes:" tag
---
 test/test/test_hash.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/test/test/test_hash.c b/test/test/test_hash.c
index b3db9fd10547..db6442a2b101 100644
--- a/test/test/test_hash.c
+++ b/test/test/test_hash.c
@@ -80,29 +80,23 @@ static uint32_t pseudo_hash(__attribute__((unused)) const void *keys,
 	return 3;
 }
 
+#define UNIT_TEST_HASH_VERBOSE	0
 /*
  * Print out result of unit test hash operation.
  */
-#if defined(UNIT_TEST_HASH_VERBOSE)
 static void print_key_info(const char *msg, const struct flow_key *key,
 								int32_t pos)
 {
-	uint8_t *p = (uint8_t *)key;
-	unsigned i;
-
-	printf("%s key:0x", msg);
-	for (i = 0; i < sizeof(struct flow_key); i++) {
-		printf("%02X", p[i]);
+	if (UNIT_TEST_HASH_VERBOSE) {
+		const uint8_t *p = (const uint8_t *)key;
+		unsigned int i;
+
+		printf("%s key:0x", msg);
+		for (i = 0; i < sizeof(struct flow_key); i++)
+			printf("%02X", p[i]);
+		printf(" @ pos %d\n", pos);
 	}
-	printf(" @ pos %d\n", pos);
-}
-#else
-static void print_key_info(__attribute__((unused)) const char *msg,
-		__attribute__((unused)) const struct flow_key *key,
-		__attribute__((unused)) int32_t pos)
-{
 }
-#endif
 
 /* Keys used by unit test functions */
 static struct flow_key keys[5] = { {
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] test/hash: solve unit test hash compilation error

[Honnappa Nagarahalli]

> 
> Enable print_key_info() function compilation always.
> 
> Fixes: af75078fece36 ("first public release")
> Cc: stable@dpdk.org
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
> v2:
> * Fix checkpatch coding style issue
> * Add "Fixes:" tag
> ---
>  test/test/test_hash.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/test/test/test_hash.c b/test/test/test_hash.c index
> b3db9fd10547..db6442a2b101 100644
> --- a/test/test/test_hash.c
> +++ b/test/test/test_hash.c
> @@ -80,29 +80,23 @@ static uint32_t pseudo_hash(__attribute__((unused))
> const void *keys,
>  	return 3;
>  }
> 
> +#define UNIT_TEST_HASH_VERBOSE	0
>  /*
>   * Print out result of unit test hash operation.
>   */
> -#if defined(UNIT_TEST_HASH_VERBOSE)
>  static void print_key_info(const char *msg, const struct flow_key *key,
>  								int32_t pos)
>  {
> -	uint8_t *p = (uint8_t *)key;
> -	unsigned i;
> -
> -	printf("%s key:0x", msg);
> -	for (i = 0; i < sizeof(struct flow_key); i++) {
> -		printf("%02X", p[i]);
> +	if (UNIT_TEST_HASH_VERBOSE) {
> +		const uint8_t *p = (const uint8_t *)key;
> +		unsigned int i;
> +
> +		printf("%s key:0x", msg);
> +		for (i = 0; i < sizeof(struct flow_key); i++)
> +			printf("%02X", p[i]);
> +		printf(" @ pos %d\n", pos);
>  	}
> -	printf(" @ pos %d\n", pos);
> -}
> -#else
> -static void print_key_info(__attribute__((unused)) const char *msg,
> -		__attribute__((unused)) const struct flow_key *key,
> -		__attribute__((unused)) int32_t pos)
> -{
>  }
> -#endif
> 
>  /* Keys used by unit test functions */
>  static struct flow_key keys[5] = { {
> --
> 2.7.4
Acked-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Thomas Monjalon]

+Cc Yipeng

18/09/2018 21:22, Dharmik Thakkar:
> Enable print_key_info() function compilation always.

Please see my first comment: you need to show the compilation error
in this message. Otherwise, we don't know what you are trying
to fix.

> Fixes: af75078fece36 ("first public release")
> Cc: stable@dpdk.org
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
> v2:
> * Fix checkpatch coding style issue
> * Add "Fixes:" tag
> ---
>  test/test/test_hash.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/test/test/test_hash.c b/test/test/test_hash.c
> index b3db9fd10547..db6442a2b101 100644
> --- a/test/test/test_hash.c
> +++ b/test/test/test_hash.c
> +#define UNIT_TEST_HASH_VERBOSE	0
>  /*
>   * Print out result of unit test hash operation.
>   */
> -#if defined(UNIT_TEST_HASH_VERBOSE)
>  static void print_key_info(const char *msg, const struct flow_key *key,
>  								int32_t pos)
>  {
> -	uint8_t *p = (uint8_t *)key;
> -	unsigned i;
> -
> -	printf("%s key:0x", msg);
> -	for (i = 0; i < sizeof(struct flow_key); i++) {
> -		printf("%02X", p[i]);
> +	if (UNIT_TEST_HASH_VERBOSE) {

This is very suspicious.
Why keeping this code if it is never called?

> +		const uint8_t *p = (const uint8_t *)key;
> +		unsigned int i;
> +
> +		printf("%s key:0x", msg);
> +		for (i = 0; i < sizeof(struct flow_key); i++)
> +			printf("%02X", p[i]);
> +		printf(" @ pos %d\n", pos);
>  	}
> -	printf(" @ pos %d\n", pos);
> -}
> -#else
> -static void print_key_info(__attribute__((unused)) const char *msg,
> -		__attribute__((unused)) const struct flow_key *key,
> -		__attribute__((unused)) int32_t pos)
> -{
>  }
> -#endif

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Wang, Yipeng1]

>-----Original Message-----
>From: Thomas Monjalon [mailto:thomas@monjalon.net]
>Sent: Friday, October 26, 2018 1:25 PM
>To: Dharmik Thakkar <dharmik.thakkar@arm.com>
>Cc: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org;
>honnappa.nagarahalli@arm.com; Wang, Yipeng1 <yipeng1.wang@intel.com>
>Subject: Re: [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error
>
>+Cc Yipeng
>
>18/09/2018 21:22, Dharmik Thakkar:
>> Enable print_key_info() function compilation always.
>
>Please see my first comment: you need to show the compilation error
>in this message. Otherwise, we don't know what you are trying
>to fix.
>
>> Fixes: af75078fece36 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
>> ---
>> v2:
>> * Fix checkpatch coding style issue
>> * Add "Fixes:" tag
>> ---
>>  test/test/test_hash.c | 24 +++++++++---------------
>>  1 file changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/test/test/test_hash.c b/test/test/test_hash.c
>> index b3db9fd10547..db6442a2b101 100644
>> --- a/test/test/test_hash.c
>> +++ b/test/test/test_hash.c
>> +#define UNIT_TEST_HASH_VERBOSE	0
>>  /*
>>   * Print out result of unit test hash operation.
>>   */
>> -#if defined(UNIT_TEST_HASH_VERBOSE)
>>  static void print_key_info(const char *msg, const struct flow_key *key,
>>  								int32_t pos)
>>  {
>> -	uint8_t *p = (uint8_t *)key;
>> -	unsigned i;
>> -
>> -	printf("%s key:0x", msg);
>> -	for (i = 0; i < sizeof(struct flow_key); i++) {
>> -		printf("%02X", p[i]);
>> +	if (UNIT_TEST_HASH_VERBOSE) {
>
>This is very suspicious.
>Why keeping this code if it is never called?

 [Wang, Yipeng] I assume this is for the convenience for debug. E.g. if the unit test failed,
developer can set the macro and print more information, but by default the code is not used.

A quick grep I found  the test_timer_racecond and efd unit test has similar macros. But could anyone
let me know what is the best coding practice for such purpose in unit test?

I also wonder what compilation error the original code causes as Thomas indicated.

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Dharmik Thakkar]

> On Oct 26, 2018, at 4:05 PM, Wang, Yipeng1 <yipeng1.wang@intel.com> wrote:
> 
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Friday, October 26, 2018 1:25 PM
>> To: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org;
>> honnappa.nagarahalli@arm.com; Wang, Yipeng1 <yipeng1.wang@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error
>> 
>> +Cc Yipeng
>> 
>> 18/09/2018 21:22, Dharmik Thakkar:
>>> Enable print_key_info() function compilation always.
>> 
>> Please see my first comment: you need to show the compilation error
>> in this message. Otherwise, we don't know what you are trying
>> to fix.
>> 
>>> Fixes: af75078fece36 ("first public release")
>>> Cc: stable@dpdk.org
>>> 
>>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
>>> ---
>>> v2:
>>> * Fix checkpatch coding style issue
>>> * Add "Fixes:" tag
>>> ---
>>> test/test/test_hash.c | 24 +++++++++---------------
>>> 1 file changed, 9 insertions(+), 15 deletions(-)
>>> 
>>> diff --git a/test/test/test_hash.c b/test/test/test_hash.c
>>> index b3db9fd10547..db6442a2b101 100644
>>> --- a/test/test/test_hash.c
>>> +++ b/test/test/test_hash.c
>>> +#define UNIT_TEST_HASH_VERBOSE	0
>>> /*
>>>  * Print out result of unit test hash operation.
>>>  */
>>> -#if defined(UNIT_TEST_HASH_VERBOSE)
>>> static void print_key_info(const char *msg, const struct flow_key *key,
>>> 								int32_t pos)
>>> {
>>> -	uint8_t *p = (uint8_t *)key;
>>> -	unsigned i;
>>> -
>>> -	printf("%s key:0x", msg);
>>> -	for (i = 0; i < sizeof(struct flow_key); i++) {
>>> -		printf("%02X", p[i]);
>>> +	if (UNIT_TEST_HASH_VERBOSE) {
>> 
>> This is very suspicious.
>> Why keeping this code if it is never called?
> 
> [Wang, Yipeng] I assume this is for the convenience for debug. E.g. if the unit test failed,
> developer can set the macro and print more information, but by default the code is not used.
> 
> A quick grep I found  the test_timer_racecond and efd unit test has similar macros. But could anyone
> let me know what is the best coding practice for such purpose in unit test?
Thank you bringing up the discussion, Yipeng. I, too, would like to know the best coding practice for such purposes.

One disadvantage of such macros is: That section of the code is only compiled when the macro is defined. 
For eg., previously, ‘print_key_info()’ did not compile without defining UNIT_TEST_HASH_VERBOSE.
Thus, it’s compilation error(s) are not accounted for always.
> 
> I also wonder what compilation error the original code causes as Thomas indicated.

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Thomas Monjalon]

26/10/2018 23:55, Dharmik Thakkar:
> 
> > On Oct 26, 2018, at 4:05 PM, Wang, Yipeng1 <yipeng1.wang@intel.com> wrote:
> > 
> >> -----Original Message-----
> >> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >> Sent: Friday, October 26, 2018 1:25 PM
> >> To: Dharmik Thakkar <dharmik.thakkar@arm.com>
> >> Cc: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org;
> >> honnappa.nagarahalli@arm.com; Wang, Yipeng1 <yipeng1.wang@intel.com>
> >> Subject: Re: [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error
> >> 
> >> +Cc Yipeng
> >> 
> >> 18/09/2018 21:22, Dharmik Thakkar:
> >>> Enable print_key_info() function compilation always.
> >> 
> >> Please see my first comment: you need to show the compilation error
> >> in this message. Otherwise, we don't know what you are trying
> >> to fix.
> >> 
> >>> Fixes: af75078fece36 ("first public release")
> >>> Cc: stable@dpdk.org
> >>> 
> >>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> >>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> >>> ---
> >>> v2:
> >>> * Fix checkpatch coding style issue
> >>> * Add "Fixes:" tag
> >>> ---
> >>> test/test/test_hash.c | 24 +++++++++---------------
> >>> 1 file changed, 9 insertions(+), 15 deletions(-)
> >>> 
> >>> diff --git a/test/test/test_hash.c b/test/test/test_hash.c
> >>> index b3db9fd10547..db6442a2b101 100644
> >>> --- a/test/test/test_hash.c
> >>> +++ b/test/test/test_hash.c
> >>> +#define UNIT_TEST_HASH_VERBOSE	0
> >>> /*
> >>>  * Print out result of unit test hash operation.
> >>>  */
> >>> -#if defined(UNIT_TEST_HASH_VERBOSE)
> >>> static void print_key_info(const char *msg, const struct flow_key *key,
> >>> 								int32_t pos)
> >>> {
> >>> -	uint8_t *p = (uint8_t *)key;
> >>> -	unsigned i;
> >>> -
> >>> -	printf("%s key:0x", msg);
> >>> -	for (i = 0; i < sizeof(struct flow_key); i++) {
> >>> -		printf("%02X", p[i]);
> >>> +	if (UNIT_TEST_HASH_VERBOSE) {
> >> 
> >> This is very suspicious.
> >> Why keeping this code if it is never called?
> > 
> > [Wang, Yipeng] I assume this is for the convenience for debug. E.g. if the unit test failed,
> > developer can set the macro and print more information, but by default the code is not used.
> > 
> > A quick grep I found  the test_timer_racecond and efd unit test has similar macros. But could anyone
> > let me know what is the best coding practice for such purpose in unit test?
> Thank you bringing up the discussion, Yipeng. I, too, would like to know the best coding practice for such purposes.
> 
> One disadvantage of such macros is: That section of the code is only compiled when the macro is defined. 
> For eg., previously, ‘print_key_info()’ did not compile without defining UNIT_TEST_HASH_VERBOSE.
> Thus, it’s compilation error(s) are not accounted for always.

The compilation time options are generally bad.
In this case, we could use the log level as a condition for printing.

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Honnappa Nagarahalli]

> > >> Subject: Re: [dpdk-stable] [PATCH v2] test/hash: solve unit test
> > >> hash compilation error
> > >>
> > >> +Cc Yipeng
> > >>
> > >> 18/09/2018 21:22, Dharmik Thakkar:
> > >>> Enable print_key_info() function compilation always.
> > >>
> > >> Please see my first comment: you need to show the compilation error
> > >> in this message. Otherwise, we don't know what you are trying to
> > >> fix.
> > >>
> > >>> Fixes: af75078fece36 ("first public release")
> > >>> Cc: stable@dpdk.org
> > >>>
> > >>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > >>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> > >>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > >>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > >>> ---
> > >>> v2:
> > >>> * Fix checkpatch coding style issue
> > >>> * Add "Fixes:" tag
> > >>> ---
> > >>> test/test/test_hash.c | 24 +++++++++---------------
> > >>> 1 file changed, 9 insertions(+), 15 deletions(-)
> > >>>
> > >>> diff --git a/test/test/test_hash.c b/test/test/test_hash.c index
> > >>> b3db9fd10547..db6442a2b101 100644
> > >>> --- a/test/test/test_hash.c
> > >>> +++ b/test/test/test_hash.c
> > >>> +#define UNIT_TEST_HASH_VERBOSE	0
> > >>> /*
> > >>>  * Print out result of unit test hash operation.
> > >>>  */
> > >>> -#if defined(UNIT_TEST_HASH_VERBOSE) static void
> > >>> print_key_info(const char *msg, const struct flow_key *key,
> > >>> 								int32_t pos)
> > >>> {
> > >>> -	uint8_t *p = (uint8_t *)key;
> > >>> -	unsigned i;
> > >>> -
> > >>> -	printf("%s key:0x", msg);
> > >>> -	for (i = 0; i < sizeof(struct flow_key); i++) {
> > >>> -		printf("%02X", p[i]);
> > >>> +	if (UNIT_TEST_HASH_VERBOSE) {
> > >>
> > >> This is very suspicious.
> > >> Why keeping this code if it is never called?
> > >
> > > [Wang, Yipeng] I assume this is for the convenience for debug. E.g.
> > > if the unit test failed, developer can set the macro and print more
> information, but by default the code is not used.
> > >
> > > A quick grep I found  the test_timer_racecond and efd unit test has
> > > similar macros. But could anyone let me know what is the best coding
> practice for such purpose in unit test?
> > Thank you bringing up the discussion, Yipeng. I, too, would like to know the
> best coding practice for such purposes.
> >
> > One disadvantage of such macros is: That section of the code is only
> compiled when the macro is defined.
> > For eg., previously, ‘print_key_info()’ did not compile without defining
> UNIT_TEST_HASH_VERBOSE.
> > Thus, it’s compilation error(s) are not accounted for always.
> 
> The compilation time options are generally bad.
> In this case, we could use the log level as a condition for printing.
This is test code. So, printing the extra log under a separate flag like UNIT_TEST_HASH_VERBOSE is ok.
When I was debugging the code, I enabled it and it did not compile. This patch ensures that the code is compiled always. But the extra logs are printed only when UNIT_TEST_HASH_VERBOSE is set to non-zero.

> 

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Thomas Monjalon]

29/10/2018 05:16, Honnappa Nagarahalli:
> > > >> Subject: Re: [dpdk-stable] [PATCH v2] test/hash: solve unit test
> > > >> hash compilation error
> > > >>
> > > >> +Cc Yipeng
> > > >>
> > > >> 18/09/2018 21:22, Dharmik Thakkar:
> > > >>> Enable print_key_info() function compilation always.
> > > >>
> > > >> Please see my first comment: you need to show the compilation error
> > > >> in this message. Otherwise, we don't know what you are trying to
> > > >> fix.
> > > >>
> > > >>> Fixes: af75078fece36 ("first public release")
> > > >>> Cc: stable@dpdk.org
> > > >>>
> > > >>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > >>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> > > >>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > >>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > >>> ---
> > > >>> v2:
> > > >>> * Fix checkpatch coding style issue
> > > >>> * Add "Fixes:" tag
> > > >>> ---
> > > >>> test/test/test_hash.c | 24 +++++++++---------------
> > > >>> 1 file changed, 9 insertions(+), 15 deletions(-)
> > > >>>
> > > >>> diff --git a/test/test/test_hash.c b/test/test/test_hash.c index
> > > >>> b3db9fd10547..db6442a2b101 100644
> > > >>> --- a/test/test/test_hash.c
> > > >>> +++ b/test/test/test_hash.c
> > > >>> +#define UNIT_TEST_HASH_VERBOSE	0
> > > >>> /*
> > > >>>  * Print out result of unit test hash operation.
> > > >>>  */
> > > >>> -#if defined(UNIT_TEST_HASH_VERBOSE) static void
> > > >>> print_key_info(const char *msg, const struct flow_key *key,
> > > >>> 								int32_t pos)
> > > >>> {
> > > >>> -	uint8_t *p = (uint8_t *)key;
> > > >>> -	unsigned i;
> > > >>> -
> > > >>> -	printf("%s key:0x", msg);
> > > >>> -	for (i = 0; i < sizeof(struct flow_key); i++) {
> > > >>> -		printf("%02X", p[i]);
> > > >>> +	if (UNIT_TEST_HASH_VERBOSE) {
> > > >>
> > > >> This is very suspicious.
> > > >> Why keeping this code if it is never called?
> > > >
> > > > [Wang, Yipeng] I assume this is for the convenience for debug. E.g.
> > > > if the unit test failed, developer can set the macro and print more
> > information, but by default the code is not used.
> > > >
> > > > A quick grep I found  the test_timer_racecond and efd unit test has
> > > > similar macros. But could anyone let me know what is the best coding
> > practice for such purpose in unit test?
> > > Thank you bringing up the discussion, Yipeng. I, too, would like to know the
> > best coding practice for such purposes.
> > >
> > > One disadvantage of such macros is: That section of the code is only
> > compiled when the macro is defined.
> > > For eg., previously, ‘print_key_info()’ did not compile without defining
> > UNIT_TEST_HASH_VERBOSE.
> > > Thus, it’s compilation error(s) are not accounted for always.
> > 
> > The compilation time options are generally bad.
> > In this case, we could use the log level as a condition for printing.
> This is test code. So, printing the extra log under a separate flag like UNIT_TEST_HASH_VERBOSE is ok.
> When I was debugging the code, I enabled it and it did not compile. This patch ensures that the code is compiled always. But the extra logs are printed only when UNIT_TEST_HASH_VERBOSE is set to non-zero.

If you keep a compilation time flag, there is a big chance that it becomes
buggy again.

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Honnappa Nagarahalli]

> 
> 29/10/2018 05:16, Honnappa Nagarahalli:
> > > > >> Subject: Re: [dpdk-stable] [PATCH v2] test/hash: solve unit
> > > > >> test hash compilation error
> > > > >>
> > > > >> +Cc Yipeng
> > > > >>
> > > > >> 18/09/2018 21:22, Dharmik Thakkar:
> > > > >>> Enable print_key_info() function compilation always.
> > > > >>
> > > > >> Please see my first comment: you need to show the compilation
> > > > >> error in this message. Otherwise, we don't know what you are
> > > > >> trying to fix.
> > > > >>
> > > > >>> Fixes: af75078fece36 ("first public release")
> > > > >>> Cc: stable@dpdk.org
> > > > >>>
> > > > >>> Suggested-by: Honnappa Nagarahalli
> > > > >>> <honnappa.nagarahalli@arm.com>
> > > > >>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> > > > >>> Reviewed-by: Honnappa Nagarahalli
> > > > >>> <honnappa.nagarahalli@arm.com>
> > > > >>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > >>> ---
> > > > >>> v2:
> > > > >>> * Fix checkpatch coding style issue
> > > > >>> * Add "Fixes:" tag
> > > > >>> ---
> > > > >>> test/test/test_hash.c | 24 +++++++++---------------
> > > > >>> 1 file changed, 9 insertions(+), 15 deletions(-)
> > > > >>>
> > > > >>> diff --git a/test/test/test_hash.c b/test/test/test_hash.c
> > > > >>> index
> > > > >>> b3db9fd10547..db6442a2b101 100644
> > > > >>> --- a/test/test/test_hash.c
> > > > >>> +++ b/test/test/test_hash.c
> > > > >>> +#define UNIT_TEST_HASH_VERBOSE	0
> > > > >>> /*
> > > > >>>  * Print out result of unit test hash operation.
> > > > >>>  */
> > > > >>> -#if defined(UNIT_TEST_HASH_VERBOSE) static void
> > > > >>> print_key_info(const char *msg, const struct flow_key *key,
> > > > >>>
> 	int32_t pos)
> > > > >>> {
> > > > >>> -	uint8_t *p = (uint8_t *)key;
> > > > >>> -	unsigned i;
> > > > >>> -
> > > > >>> -	printf("%s key:0x", msg);
> > > > >>> -	for (i = 0; i < sizeof(struct flow_key); i++) {
> > > > >>> -		printf("%02X", p[i]);
> > > > >>> +	if (UNIT_TEST_HASH_VERBOSE) {
> > > > >>
> > > > >> This is very suspicious.
> > > > >> Why keeping this code if it is never called?
> > > > >
> > > > > [Wang, Yipeng] I assume this is for the convenience for debug. E.g.
> > > > > if the unit test failed, developer can set the macro and print
> > > > > more
> > > information, but by default the code is not used.
> > > > >
> > > > > A quick grep I found  the test_timer_racecond and efd unit test
> > > > > has similar macros. But could anyone let me know what is the
> > > > > best coding
> > > practice for such purpose in unit test?
> > > > Thank you bringing up the discussion, Yipeng. I, too, would like
> > > > to know the
> > > best coding practice for such purposes.
> > > >
> > > > One disadvantage of such macros is: That section of the code is
> > > > only
> > > compiled when the macro is defined.
> > > > For eg., previously, ‘print_key_info()’ did not compile without
> > > > defining
> > > UNIT_TEST_HASH_VERBOSE.
> > > > Thus, it’s compilation error(s) are not accounted for always.
> > >
> > > The compilation time options are generally bad.
> > > In this case, we could use the log level as a condition for printing.
> > This is test code. So, printing the extra log under a separate flag like
> UNIT_TEST_HASH_VERBOSE is ok.
> > When I was debugging the code, I enabled it and it did not compile. This
> patch ensures that the code is compiled always. But the extra logs are printed
> only when UNIT_TEST_HASH_VERBOSE is set to non-zero.
> 
> If you keep a compilation time flag, there is a big chance that it becomes
> buggy again.
> 
I believe you meant, buggy during run time. If yes, I agree. This patch removes only compile time bugs.
Looks like using 'rte_log_set_level', one can enable the logs only for hash library. So, it can be used for unit tests as well rather than defining another flag.
However, it will not solve the run time bugs as the log level will be set to not print the info by default. IMO, it does not make sense to print this all the time.

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Dharmik Thakkar]

> On Oct 26, 2018, at 3:24 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> +Cc Yipeng
> 
> 18/09/2018 21:22, Dharmik Thakkar:
>> Enable print_key_info() function compilation always.
> 
> Please see my first comment: you need to show the compilation error
> in this message. Otherwise, we don't know what you are trying
> to fix.
Sure, I will submit a new version with the compilation error message.
> 
>> Fixes: af75078fece36 ("first public release")
>> Cc: stable@dpdk.org
>> 
>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
>> ---
>> v2:
>> * Fix checkpatch coding style issue
>> * Add "Fixes:" tag
>> ---
>> test/test/test_hash.c | 24 +++++++++---------------
>> 1 file changed, 9 insertions(+), 15 deletions(-)
>> 
>> diff --git a/test/test/test_hash.c b/test/test/test_hash.c
>> index b3db9fd10547..db6442a2b101 100644
>> --- a/test/test/test_hash.c
>> +++ b/test/test/test_hash.c
>> +#define UNIT_TEST_HASH_VERBOSE	0
>> /*
>>  * Print out result of unit test hash operation.
>>  */
>> -#if defined(UNIT_TEST_HASH_VERBOSE)
>> static void print_key_info(const char *msg, const struct flow_key *key,
>> 								int32_t pos)
>> {
>> -	uint8_t *p = (uint8_t *)key;
>> -	unsigned i;
>> -
>> -	printf("%s key:0x", msg);
>> -	for (i = 0; i < sizeof(struct flow_key); i++) {
>> -		printf("%02X", p[i]);
>> +	if (UNIT_TEST_HASH_VERBOSE) {
> 
> This is very suspicious.
> Why keeping this code if it is never called?
The function ‘print_key_info’ is being called at different places and developer can set the macro, to print debug information.
> 
>> +		const uint8_t *p = (const uint8_t *)key;
>> +		unsigned int i;
>> +
>> +		printf("%s key:0x", msg);
>> +		for (i = 0; i < sizeof(struct flow_key); i++)
>> +			printf("%02X", p[i]);
>> +		printf(" @ pos %d\n", pos);
>> 	}
>> -	printf(" @ pos %d\n", pos);
>> -}
>> -#else
>> -static void print_key_info(__attribute__((unused)) const char *msg,
>> -		__attribute__((unused)) const struct flow_key *key,
>> -		__attribute__((unused)) int32_t pos)
>> -{
>> }
>> -#endif

[dpdk-dev] [PATCH v3] test/hash: solve unit test hash compilation error

[Dharmik Thakkar]

Enable print_key_info() function compilation always.

Compilation error message:
'test_hash.c: In function ‘print_key_info’:
test_hash.c:90:15: error: cast discards ‘const’ qualifier from pointer
target type [-Werror=cast-qual]
  uint8_t *p = (uint8_t *)key;
               ^
cc1: all warnings being treated as errors'


Fixes: af75078fece36 ("first public release")
Cc: stable@dpdk.org

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
v3:
* Add compilation error message
---
v2:
* Fix checkpatch coding style issue
* Add "Fixes:" tag
---
 test/test/test_hash.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/test/test/test_hash.c b/test/test/test_hash.c
index b3db9fd10547..db6442a2b101 100644
--- a/test/test/test_hash.c
+++ b/test/test/test_hash.c
@@ -80,29 +80,23 @@ static uint32_t pseudo_hash(__attribute__((unused)) const void *keys,
 	return 3;
 }
 
+#define UNIT_TEST_HASH_VERBOSE	0
 /*
  * Print out result of unit test hash operation.
  */
-#if defined(UNIT_TEST_HASH_VERBOSE)
 static void print_key_info(const char *msg, const struct flow_key *key,
 								int32_t pos)
 {
-	uint8_t *p = (uint8_t *)key;
-	unsigned i;
-
-	printf("%s key:0x", msg);
-	for (i = 0; i < sizeof(struct flow_key); i++) {
-		printf("%02X", p[i]);
+	if (UNIT_TEST_HASH_VERBOSE) {
+		const uint8_t *p = (const uint8_t *)key;
+		unsigned int i;
+
+		printf("%s key:0x", msg);
+		for (i = 0; i < sizeof(struct flow_key); i++)
+			printf("%02X", p[i]);
+		printf(" @ pos %d\n", pos);
 	}
-	printf(" @ pos %d\n", pos);
-}
-#else
-static void print_key_info(__attribute__((unused)) const char *msg,
-		__attribute__((unused)) const struct flow_key *key,
-		__attribute__((unused)) int32_t pos)
-{
 }
-#endif
 
 /* Keys used by unit test functions */
 static struct flow_key keys[5] = { {
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] test/hash: solve unit test hash compilation error

[Thomas Monjalon]

26/10/2018 23:43, Dharmik Thakkar:
> Enable print_key_info() function compilation always.
> 
> Compilation error message:
> 'test_hash.c: In function ‘print_key_info’:
> test_hash.c:90:15: error: cast discards ‘const’ qualifier from pointer
> target type [-Werror=cast-qual]
>   uint8_t *p = (uint8_t *)key;
>                ^
> cc1: all warnings being treated as errors'
> 
> 
> Fixes: af75078fece36 ("first public release")
> Cc: stable@dpdk.org
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>

Applied, thanks