Message ID | 20240527222947.374475-1-pvorel@suse.cz |
---|---|
Headers | show |
Series | lib: Add TINFO_WARN | expand |
Hi! I'm not sure about this. Why not enabling TINFO + TWARN combination instead? tst_res(TINFO | TWARN, "my message"); On 5/28/24 00:29, Petr Vorel wrote: > Hi, > > if we found this useful, is it worth to implement it also in the shell > API? > > Regardless the result, should it be TDEBUG backported to the shell API? > > Kind regards, > Petr > > Petr Vorel (2): > lib: Add TINFO_WARN > tree: Use TINFO_WARN > > include/tst_res_flags.h | 6 +++++- > include/tst_test.h | 2 +- > lib/newlib_tests/tst_res_flags.c | 1 + > lib/tst_ansi_color.c | 1 + > lib/tst_res.c | 12 ++++++++---- > lib/tst_supported_fs_types.c | 2 +- > lib/tst_test.c | 3 +++ > testcases/kernel/mem/hugetlb/lib/hugetlb.c | 4 ++-- > testcases/kernel/syscalls/ipc/semctl/semctl08.c | 2 +- > 9 files changed, 23 insertions(+), 10 deletions(-) > Andrea
Hi! > I'm not sure about this. Why not enabling TINFO + TWARN combination instead? > > tst_res(TINFO | TWARN, "my message"); That's even more confusing. Does that propagate into results or not? To be honest we even have problem deciding if we should use TINFO or TWARN in some cases and adding third variant would make things even worse, sicne we would have three options. So I would really keep just TINFO which is something that is printed by default but does not propagate into results and TWARN that is printed as well but propagates into results. Maybe it would be even better to actually remove TWARN. That way we would have only TFAIL and TBROK that propagate into results since TWARN is kind of lesser TBROK anyways...
> Hi! > > I'm not sure about this. Why not enabling TINFO + TWARN combination instead? > > tst_res(TINFO | TWARN, "my message"); > That's even more confusing. Does that propagate into results or not? > To be honest we even have problem deciding if we should use TINFO or > TWARN in some cases and adding third variant would make things even > worse, sicne we would have three options. > So I would really keep just TINFO which is something that is printed by > default but does not propagate into results and TWARN that is printed as > well but propagates into results. > Maybe it would be even better to actually remove TWARN. That way we > would have only TFAIL and TBROK that propagate into results since TWARN > is kind of lesser TBROK anyways... Understand to your points. But tst_res TWARN is quite understandable (simple warning, which propagates), IMHO better to use than tst_res TBROK. But sure, feel free to go ahead and send a patch to remove TWARN. BTW I was also surprised how many tests use tst_resm TBROK ... / tst_resm(TBROK, ...) followed by exit 1 / exit(1). These should be converted to tst_brkm TBROK / tst_resm(TBROK, ...). Other thing is, that I would prefer to have macro for tst_res(TINFO, "WARNING: ...") e.g. WARNING(...) would produce tst_res(TINFO, "WARNING: ...") from simple point I don't like to hardwire text (there can be typos). BTW more than for this rare case I would prefer to have macros for .tags, e.g. LINUX_GIT(43a6684519ab) would produce {"linux-git", "43a6684519ab"} CVE(2017-2671) would produce {"CVE", "2017-2671"} (again, typos). Kind regards, Petr
Hi Petr, All, On Thu, Jun 20, 2024 at 1:36 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi! > > > I'm not sure about this. Why not enabling TINFO + TWARN combination > instead? > > > > tst_res(TINFO | TWARN, "my message"); > > > That's even more confusing. Does that propagate into results or not? > > > To be honest we even have problem deciding if we should use TINFO or > > TWARN in some cases and adding third variant would make things even > > worse, sicne we would have three options. > > > So I would really keep just TINFO which is something that is printed by > > default but does not propagate into results and TWARN that is printed as > > well but propagates into results. > > > Maybe it would be even better to actually remove TWARN. That way we > > would have only TFAIL and TBROK that propagate into results since TWARN > > is kind of lesser TBROK anyways... > > Understand to your points. But tst_res TWARN is quite understandable > (simple warning, which propagates), IMHO better to use than > tst_res TBROK. But sure, feel free to go ahead and send a patch to remove > TWARN. > +1 to remove TWARN. > BTW I was also surprised how many tests use tst_resm TBROK ... / > tst_resm(TBROK, > ...) followed by exit 1 / exit(1). These should be converted to tst_brkm > TBROK / > tst_resm(TBROK, ...). > > Other thing is, that I would prefer to have macro for tst_res(TINFO, > "WARNING: ...") > e.g. WARNING(...) would produce tst_res(TINFO, "WARNING: ...") from simple > point I > don't like to hardwire text (there can be typos). BTW more than for this > rare > case I would prefer to have macros for .tags, e.g. LINUX_GIT(43a6684519ab) > would > produce {"linux-git", "43a6684519ab"} CVE(2017-2671) would produce {"CVE", > "2017-2671"} > (again, typos). > What about defining new LTP macros like TST_RES_TINFO(), TST_RES_TFAIL(), ... TST_RES_WARNING(), so that we can save time on inputting the TFAIL/TINFO keywords every time? also TST_LINUX_GIT(), TST_CVE() ?
> Hi Petr, All, > On Thu, Jun 20, 2024 at 1:36 PM Petr Vorel <pvorel@suse.cz> wrote: > > > Hi! > > > > I'm not sure about this. Why not enabling TINFO + TWARN combination > > instead? > > > > tst_res(TINFO | TWARN, "my message"); > > > That's even more confusing. Does that propagate into results or not? > > > To be honest we even have problem deciding if we should use TINFO or > > > TWARN in some cases and adding third variant would make things even > > > worse, sicne we would have three options. > > > So I would really keep just TINFO which is something that is printed by > > > default but does not propagate into results and TWARN that is printed as > > > well but propagates into results. > > > Maybe it would be even better to actually remove TWARN. That way we > > > would have only TFAIL and TBROK that propagate into results since TWARN > > > is kind of lesser TBROK anyways... > > Understand to your points. But tst_res TWARN is quite understandable > > (simple warning, which propagates), IMHO better to use than > > tst_res TBROK. But sure, feel free to go ahead and send a patch to remove > > TWARN. > +1 to remove TWARN. > > BTW I was also surprised how many tests use tst_resm TBROK ... / > > tst_resm(TBROK, > > ...) followed by exit 1 / exit(1). These should be converted to tst_brkm > > TBROK / > > tst_resm(TBROK, ...). > > Other thing is, that I would prefer to have macro for tst_res(TINFO, > > "WARNING: ...") > > e.g. WARNING(...) would produce tst_res(TINFO, "WARNING: ...") from simple > > point I > > don't like to hardwire text (there can be typos). BTW more than for this > > rare > > case I would prefer to have macros for .tags, e.g. LINUX_GIT(43a6684519ab) > > would > > produce {"linux-git", "43a6684519ab"} CVE(2017-2671) would produce {"CVE", > > "2017-2671"} > > (again, typos). > What about defining new LTP macros like > TST_RES_TINFO(), > TST_RES_TFAIL(), > ... > TST_RES_WARNING(), > so that we can save time on inputting the TFAIL/TINFO keywords every time? > also TST_LINUX_GIT(), TST_CVE() ? +1
Hi All,
I come up with a way to define simple macros for all LTP print, especially
for TFAIL
we could make use of the GET_MACRO to select a specific macro implementation
based on the number of arguments provided to a variadic macro.
Something partly like:
--- a/include/tst_test_macros.h
+++ b/include/tst_test_macros.h
@@ -368,4 +368,28 @@ const char *tst_errno_names(char *buf, const int
*exp_errs, int exp_errs_cnt);
#define TST_EXP_EQ_SSZ(VAL_A, VAL_B) \
TST_EXP_EQ_(VAL_A, #VAL_A, VAL_B, #VAL_B, ssize_t, "%zi")
+/*TST_RES*/
+#define TST_RES_TINFO(MESSAGE) \
+ tst_res(TINFO, MESSAGE);
+
+#define TST_RES_TWARN(MESSAGE) \
+ tst_res(TINFO, "WARNING "MESSAGE);
+
+#define TST_RES_TCONF(MESSAGE) \
+ tst_res(TCONF, MESSAGE);
+
+#define TST_RES_TPASS(MESSAGE) \
+ tst_res(TPASS, MESSAGE);
+
+#define TST_RES_TFAIL(...) \
+ GET_MACRO(__VA_ARGS__, TST_RES_FAIL2,
TST_RES_FAIL1)(__VA_ARGS__)
+
+#define GET_MACRO(_1, _2, NAME, ...) NAME
+
+#define TST_RES_FAIL1(MESSAGE) \
+ tst_res(TFAIL, MESSAGE)
+
+#define TST_RES_FAIL2(flag, MESSAGE) \
+ tst_res(TFAIL | flag, MESSAGE)
+
#endif /* TST_TEST_MACROS_H__ */
diff --git a/lib/newlib_tests/tst_res_macros.c
b/lib/newlib_tests/tst_res_macros.c
new file mode 100644
index 000000000..e16f3cbba
--- /dev/null
+++ b/lib/newlib_tests/tst_res_macros.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2024 Li Wang <liwang@redhat.com>
+ */
+
+#include "tst_test.h"
+#include "tst_res_flags.h"
+#include "tst_test_macros.h"
+
+static void do_test(unsigned int i)
+{
+ TST_RES_TINFO("message");
+ TST_RES_TPASS("message");
+ TST_RES_TWARN("message");
+ TST_RES_TFAIL("message");
+ TST_RES_TFAIL(TERRNO, "message");
+ TST_RES_TFAIL(TTERRNO, "message");
+}
+
+static struct tst_test test = {
+ .test_all = do_test,
+};
>
Hi Li, > Hi All, > I come up with a way to define simple macros for all LTP print, especially > for TFAIL > we could make use of the GET_MACRO to select a specific macro implementation > based on the number of arguments provided to a variadic macro. > Something partly like: > --- a/include/tst_test_macros.h > +++ b/include/tst_test_macros.h > @@ -368,4 +368,28 @@ const char *tst_errno_names(char *buf, const int > *exp_errs, int exp_errs_cnt); > #define TST_EXP_EQ_SSZ(VAL_A, VAL_B) \ > TST_EXP_EQ_(VAL_A, #VAL_A, VAL_B, #VAL_B, ssize_t, "%zi") > +/*TST_RES*/ > +#define TST_RES_TINFO(MESSAGE) \ > + tst_res(TINFO, MESSAGE); > + > +#define TST_RES_TWARN(MESSAGE) \ > + tst_res(TINFO, "WARNING "MESSAGE); > + > +#define TST_RES_TCONF(MESSAGE) \ > + tst_res(TCONF, MESSAGE); > + > +#define TST_RES_TPASS(MESSAGE) \ > + tst_res(TPASS, MESSAGE); > + > +#define TST_RES_TFAIL(...) \ > + GET_MACRO(__VA_ARGS__, TST_RES_FAIL2, > TST_RES_FAIL1)(__VA_ARGS__) > + > +#define GET_MACRO(_1, _2, NAME, ...) NAME > + > +#define TST_RES_FAIL1(MESSAGE) \ > + tst_res(TFAIL, MESSAGE) > + > +#define TST_RES_FAIL2(flag, MESSAGE) \ > + tst_res(TFAIL | flag, MESSAGE) > + > #endif /* TST_TEST_MACROS_H__ */ > diff --git a/lib/newlib_tests/tst_res_macros.c > b/lib/newlib_tests/tst_res_macros.c > new file mode 100644 > index 000000000..e16f3cbba > --- /dev/null > +++ b/lib/newlib_tests/tst_res_macros.c > @@ -0,0 +1,22 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2024 Li Wang <liwang@redhat.com> > + */ > + > +#include "tst_test.h" > +#include "tst_res_flags.h" > +#include "tst_test_macros.h" > + > +static void do_test(unsigned int i) > +{ > + TST_RES_TINFO("message"); > + TST_RES_TPASS("message"); > + TST_RES_TWARN("message"); > + TST_RES_TFAIL("message"); > + TST_RES_TFAIL(TERRNO, "message"); > + TST_RES_TFAIL(TTERRNO, "message"); > +} > + > +static struct tst_test test = { > + .test_all = do_test, > +}; Reviewed-by: Petr Vorel <pvorel@suse.cz> Thanks! Kind regards, Petr
On Fri, Jun 21, 2024 at 9:39 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > Hi All, > > > I come up with a way to define simple macros for all LTP print, > especially > > for TFAIL > > we could make use of the GET_MACRO to select a specific macro > implementation > > based on the number of arguments provided to a variadic macro. > > > Something partly like: > > > --- a/include/tst_test_macros.h > > +++ b/include/tst_test_macros.h > > @@ -368,4 +368,28 @@ const char *tst_errno_names(char *buf, const int > > *exp_errs, int exp_errs_cnt); > > #define TST_EXP_EQ_SSZ(VAL_A, VAL_B) \ > > TST_EXP_EQ_(VAL_A, #VAL_A, VAL_B, #VAL_B, ssize_t, "%zi") > > > +/*TST_RES*/ > > +#define TST_RES_TINFO(MESSAGE) \ > > + tst_res(TINFO, MESSAGE); > > + > > +#define TST_RES_TWARN(MESSAGE) \ > > + tst_res(TINFO, "WARNING "MESSAGE); > > + > > +#define TST_RES_TCONF(MESSAGE) \ > > + tst_res(TCONF, MESSAGE); > > + > > +#define TST_RES_TPASS(MESSAGE) \ > > + tst_res(TPASS, MESSAGE); > > + > > +#define TST_RES_TFAIL(...) \ > > + GET_MACRO(__VA_ARGS__, TST_RES_FAIL2, > > TST_RES_FAIL1)(__VA_ARGS__) > > + > > +#define GET_MACRO(_1, _2, NAME, ...) NAME > > + > > +#define TST_RES_FAIL1(MESSAGE) \ > > + tst_res(TFAIL, MESSAGE) > > + > > +#define TST_RES_FAIL2(flag, MESSAGE) \ > > + tst_res(TFAIL | flag, MESSAGE) > > + > > #endif /* TST_TEST_MACROS_H__ */ > > diff --git a/lib/newlib_tests/tst_res_macros.c > > b/lib/newlib_tests/tst_res_macros.c > > new file mode 100644 > > index 000000000..e16f3cbba > > --- /dev/null > > +++ b/lib/newlib_tests/tst_res_macros.c > > @@ -0,0 +1,22 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2024 Li Wang <liwang@redhat.com> > > + */ > > + > > +#include "tst_test.h" > > +#include "tst_res_flags.h" > > +#include "tst_test_macros.h" > > + > > +static void do_test(unsigned int i) > > +{ > > + TST_RES_TINFO("message"); > > + TST_RES_TPASS("message"); > > + TST_RES_TWARN("message"); > > + TST_RES_TFAIL("message"); > > + TST_RES_TFAIL(TERRNO, "message"); > > + TST_RES_TFAIL(TTERRNO, "message"); > > +} > > + > > +static struct tst_test test = { > > + .test_all = do_test, > > +}; > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Thanks! > Thank you so much. Note: Basically, the method works well, but contains a tiny issue that people sometimes need to print variable value in tst_res(, "val = %d", val), so it still needs refined code to resolve that. I will send out a full support patch after testing good on my side. (maybe tomorrow).
Hi Li, ... > Basically, the method works well, but contains a tiny issue that people > sometimes need to print variable value in tst_res(, "val = %d", val), > so it still needs refined code to resolve that. > I will send out a full support patch after testing good on my side. > (maybe tomorrow). ... and __VA_ARGS__ see include/tst_test_macros.h #define TST_FMT_(FMT, _1, ...) FMT, ##__VA_ARGS__ Kind regards, Petr