Re: [LTP] [PATCH v1] Fixes: #776, tst_taint prints human readable error messages instead of numerical codes

[Petr Vorel <pvorel@suse.cz>]

Hi Kushal,

> This patch prints human readable messages when kernel is tainted instead
> of numerical codes.

> Git Hub Issue link  - https://github.com/linux-test-project/ltp/issues/776

> Signed-off-by: Kushal Chand <kushalkataria5@gmail.com>

commit message could be improved, but that's a detail.
I'd put commit message as:

tst_taint: print readable error instead of numerical codes

Fixes: #776

Signed-off-by: Kushal Chand <kushalkataria5@gmail.com>

> ---
>  lib/tst_taint.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)

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

>  static unsigned int taint_mask = -1;

> +struct pair {
> +	const char *name;
> +	int val;
> +};
> +
> +#define PAIR(def)[def] = {.name = #def, .val = def},
> +
> +#define STRPAIR(key, value)[key] = {.name = value, .val = key},
> +
> +#define PAIR_LOOKUP(pair_arr, idx) do {			      \
> +	if (idx < 0 || (size_t)idx >= ARRAY_SIZE(pair_arr) || \
> +	    pair_arr[idx].name == NULL)			      \
> +		return "???";				      \
> +	return pair_arr[idx].name;			      \
> +} while (0)

You copy pasted definitions from lib/tst_res.c. It's quite a lot of code to be
duplicated. Could you please move these definitions from include/tst_common.h
into lib/tst_res.c in separate commit?

Also when you generate new version with git format-patch, please use -v2
(as a second version). Also feel free to Cc me with
--cc 'Petr Vorel <pvorel@suse.cz>'

> +
> +const char *tst_strtaint(int err)
> +{
> +	static const struct pair taint_pairs[] = {
> +		STRPAIR(TST_TAINT_A, "TAINT_A(ACPI table overridden)")
I'd really rather remove TAINT_ (that's our LTP string, IMHO not needed)
as I suggested previously. And put space after the letter. i.e.:
		STRPAIR(TST_TAINT_A, "A (ACPI table overridden)")

Kind regards,
Petr

> +		STRPAIR(TST_TAINT_B, "TAINT_B(Bad page reference)")
> +		STRPAIR(TST_TAINT_C, "TAINT_C(Staging driver loaded)")
> +		STRPAIR(TST_TAINT_D, "TAINT_D(OOPS/BUG)")
> +		STRPAIR(TST_TAINT_E, "TAINT_E(Unsigned module loaded)")
> +		STRPAIR(TST_TAINT_F, "TAINT_F(Module force loaded)")
> +		STRPAIR(TST_TAINT_G, "TAINT_G(Propriety module loaded)")
> +		STRPAIR(TST_TAINT_I, "TAINT_I(Workaround BIOS/FW bug)")
> +		STRPAIR(TST_TAINT_K, "TAINT_K(Live patched)")
> +		STRPAIR(TST_TAINT_L, "TAINT_L(Soft lock up occured)")
> +		STRPAIR(TST_TAINT_M, "TAINT_M(Machine check exception)")
> +		STRPAIR(TST_TAINT_O, "TAINT_O(Out of tree module loaded)")
> +		STRPAIR(TST_TAINT_R, "TAINT_R(Module force unloaded)")
> +		STRPAIR(TST_TAINT_S, "TAINT_S(Running on out of spec system)")
> +		STRPAIR(TST_TAINT_T, "TAINT_T(Built with struct randomization)")
> +		STRPAIR(TST_TAINT_U, "TAINT_U(User request)")
> +		STRPAIR(TST_TAINT_W, "TAINT_W(Warning)")
> +		STRPAIR(TST_TAINT_X, "TAINT_X(Auxilary)")
> +	};
> +
> +	PAIR_LOOKUP(taint_pairs, err);
> +}
> +
>  static unsigned int tst_taint_read(void)
>  {
>  	unsigned int val;
> @@ -90,7 +132,8 @@ void tst_taint_init(unsigned int mask)
>  	}

>  	if ((taint & taint_mask) != 0)
> -		tst_brk(TBROK, "Kernel is already tainted: %u", taint);
> +		tst_brk(TBROK, "Kernel is already tainted: %s",
> +			tst_strtaint(taint));
>  }

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