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

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

[Kushal Chand]

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[] = {
+				       "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)",
+};
+
 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;
 
 	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]);
 }
 
 
-- 
2.25.1


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

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

[Petr Vorel]

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

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

[Petr Vorel]

Hi Kusal,

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

I'm sorry, actually not a char array - I forgot we don't print just the letter,
but string (letter with a description).
You could accumulate string with strcat and print it at the end.
But maybe just use tst_res(TINFO, ...) to print the flag in the loop
and in the end tst_brk(TBROK, "Kernel is already tainted").

Kind regards,
Petr

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