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

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

[Kushal Chand]

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>

---
 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)
+
+const char *tst_strtaint(int err)
+{
+	static const struct pair taint_pairs[] = {
+		STRPAIR(TST_TAINT_A, "TAINT_A(ACPI table overridden)")
+		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));
 }
 
 
-- 
2.25.1


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

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

[Petr Vorel]

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

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

[Martin Doucha]

Hi,

On 14. 01. 22 17:16, Kushal Chand wrote:
> 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>
> 
> ---
>  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)
> +
> +const char *tst_strtaint(int err)
> +{
> +	static const struct pair taint_pairs[] = {
> +		STRPAIR(TST_TAINT_A, "TAINT_A(ACPI table overridden)")
> +		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);

This is not the correct approach. You've constructed an array with
131,073 items to store a total of 18 strings. And the value passed in
the "err" parameter is a bitmask which can hold multiple taint flags.

What you should do is this:

const char *taint_strings[] = {
	"G (Propriety module loaded)",
	"F (Module force loaded)",
	"S (Running on out of spec system)",
	"R (Module force unloaded)",
	...
	"X (Auxilary)",
	"T (Built with struct randomization)"
};

Then loop from 0 to ARRAY_SIZE(taint_strings) and print taint_strings[i]
if (err & (1 << i)) != 0

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

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

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

[Petr Vorel]

Hi Martin, Kushal,

...
> > +const char *tst_strtaint(int err)
> > +{
> > +	static const struct pair taint_pairs[] = {
> > +		STRPAIR(TST_TAINT_A, "TAINT_A(ACPI table overridden)")
> > +		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);

> This is not the correct approach. You've constructed an array with
> 131,073 items to store a total of 18 strings. And the value passed in
> the "err" parameter is a bitmask which can hold multiple taint flags.

> What you should do is this:

> const char *taint_strings[] = {
> 	"G (Propriety module loaded)",
> 	"F (Module force loaded)",
> 	"S (Running on out of spec system)",
> 	"R (Module force unloaded)",
> 	...
> 	"X (Auxilary)",
> 	"T (Built with struct randomization)"
> };

> Then loop from 0 to ARRAY_SIZE(taint_strings) and print taint_strings[i]
> if (err & (1 << i)) != 0

Martin thanks a lot for correcting me. Kushal, I'm sorry for wrong advice,
please follow Martin's suggestion in v3.

Kind regards,
Petr

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