| Message ID | 20220114161612.206475-1-kushalkataria5@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] Fixes: #776, tst_taint prints human readable error messages instead of numerical codes | expand |
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)); > }
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
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
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)); }
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(-)