mbox series

[0/2] lib: Add TINFO_WARN

Message ID 20240527222947.374475-1-pvorel@suse.cz
Headers show
Series lib: Add TINFO_WARN | expand

Message

Petr Vorel May 27, 2024, 10:29 p.m. UTC
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(-)

Comments

Andrea Cervesato May 28, 2024, 6:04 a.m. UTC | #1
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
Cyril Hrubis May 29, 2024, 3:10 p.m. UTC | #2
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...
Petr Vorel June 20, 2024, 5:36 a.m. UTC | #3
> 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
Li Wang June 20, 2024, 8:05 a.m. UTC | #4
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() ?
Petr Vorel June 20, 2024, 11:25 a.m. UTC | #5
> 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
Li Wang June 21, 2024, 1:22 p.m. UTC | #6
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,
+};

>
Petr Vorel June 21, 2024, 1:39 p.m. UTC | #7
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
Li Wang June 21, 2024, 1:47 p.m. UTC | #8
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).
Petr Vorel June 21, 2024, 1:53 p.m. UTC | #9
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