[LTP] [PATCH v3] tst_taint: print readable error message instead of numerical codes
Petr Vorel
pvorel@suse.cz
Thu Jan 20 21:14:40 CET 2022
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
> }
More information about the ltp
mailing list