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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

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