Re: [LTP] [PATCH v3] tst_taint: print readable error message instead of numerical codes

[Petr Vorel <pvorel@suse.cz>]

Hi Kusal,

there are one problematic thing: use tst_brk() and code style.
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2-coding-style


> This patch stores the possible kernel tainted messages in taint_strings
> and corresponding error is printed.

> Fixes: #776
> ---
>  lib/tst_taint.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)

> diff --git a/lib/tst_taint.c b/lib/tst_taint.c
> index 49146aacb..e224984f5 100644
> --- a/lib/tst_taint.c
> +++ b/lib/tst_taint.c
> @@ -8,6 +8,27 @@

>  static unsigned int taint_mask = -1;

> +static const char * const taint_strings[] = {
nit: please remove space after star
static const char *const taint_strings[] = {
> +				       "G (Propriety module loaded)",
> +				       "F (Module force loaded)",
> +				       "S (Running on out of spec system)",
> +				       "R (Module force unloaded)",
> +				       "M (Machine check exception)",
> +				       "B (Bad page reference)",
> +				       "U (User request)",
> +				       "D (OOPS/BUG)",
> +				       "A (ACPI table overridden)",
> +				       "W (Warning)",
> +				       "C (Staging driver loaded)",
> +				       "I (Workaround BIOS/FW bug)",
> +				       "O (Out of tree module loaded)",
> +				       "E (Unsigned module loaded)",
> +				       "L (Soft lock up occured)",
> +				       "K (Live patched)",
> +				       "X (Auxilary)",
> +				       "T (Built with struct randomization)",
nit: Why so many levels to indent? You also mix tabs and spaces.
Could you use just 1 tab?

> +};

> +
>  static unsigned int tst_taint_read(void)
>  {
>  	unsigned int val;
> @@ -74,6 +95,7 @@ static int tst_taint_check_kver(unsigned int mask)
>  void tst_taint_init(unsigned int mask)
>  {
>  	unsigned int taint = -1;
> +	long unsigned int i;
Please use unsigned long

NOTE: warn done by 'cd lib && make check-tst_taint'
tst_taint.c:98: WARNING: type 'long unsigned int' should be specified in [[un]signed] [short|int|long|long long] order
tst_taint.c:98: WARNING: Prefer 'unsigned long' over 'long unsigned int' as the int is unnecessary

there are more warning which you can ignore for now.

>  	if (mask == 0)
>  		tst_brk(TBROK, "mask is not allowed to be 0");
> @@ -90,7 +112,10 @@ void tst_taint_init(unsigned int mask)
>  	}

>  	if ((taint & taint_mask) != 0)
> -		tst_brk(TBROK, "Kernel is already tainted: %u", taint);
> +		for (i = 0; i < ARRAY_SIZE(taint_strings); i++)
> +			if (taint & (1 << i))
> +				tst_brk(TBROK, "Kernel is already tainted: %s",
> +					taint_strings[i]);
The main reason why I just didn't fix the whitespace myself and applied is using
tst_brk(). It quits test on first matching flag. You can accumulate letters into
char array and print after loop.

nit: Please wrap the code around for { }
https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces

FYI we have make check target to help prevent. But some info can be confusing
(not related to your changes or even false positive):

E.g. for this:

$ cd lib && make check-tst_taint
tst_taint.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
=> you can ignore this

tst_taint.c:98: WARNING: type 'long unsigned int' should be specified in [[un]signed] [short|int|long|long long] order
tst_taint.c:98: WARNING: Prefer 'unsigned long' over 'long unsigned int' as the int is unnecessary
=> please fix this one 

make: [../include/mk/rules.mk:48: check-tst_taint] Error 1 (ignored)
tst_taint.c:32:21: warning: LTP-003: Symbol 'tst_taint_read' has the LTP public library prefix, but is static (private).
tst_taint.c:41:12: warning: LTP-003: Symbol 'tst_taint_check_kver' has the LTP public library prefix, but is static (private).
=> you can ignore these two.

Both code style and make check are documented at:
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2-coding-style

>  }

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp