| Message ID | 20220120163407.30744-1-kushalkataria5@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] tst_taint: print readable error message instead of numerical codes | expand |
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 > }
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
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]); }