[RFC] lib/tst_test.c: Fix tst_brk() handling

This makes the tst_brk() handling cleaner and saner as instead of
propagating the tst_brk() result in a return value an abort flag is
introduced into the shared memory.

Now:

- All the processes but the library one that reports the results exit
  with 0
- tst_brk(TBROK, ...) increments result conters, sets the abort flag.
  and exit current process
- all other tst_brk() variants will just increments the countes and
  exits the current process

This makes the tst_brk() behavior well defined so we can now even call
tst_brk() with TFAIL and TPASS as well.

Open question (may be done in a separatep patch):

Should tst_brk(TBROK, ...) apart from setting the flag also send sigkill
signal to the test process group to kill any leftover test processes?

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 include/tst_test.h          | 24 +++++++-------
 lib/newlib_tests/.gitignore |  6 +++-
 lib/newlib_tests/test23.c   | 25 ++++++++++++++
 lib/newlib_tests/test24.c   | 25 ++++++++++++++
 lib/newlib_tests/test25.c   | 19 +++++++++++
 lib/newlib_tests/test26.c   | 20 +++++++++++
 lib/tst_test.c              | 66 +++++++++++++++++++++----------------
 7 files changed, 144 insertions(+), 41 deletions(-)
 create mode 100644 lib/newlib_tests/test23.c
 create mode 100644 lib/newlib_tests/test24.c
 create mode 100644 lib/newlib_tests/test25.c
 create mode 100644 lib/newlib_tests/test26.c

On Fri, Nov 15, 2024 at 5:41 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> This makes the tst_brk() handling cleaner and saner as instead of
> propagating the tst_brk() result in a return value an abort flag is
> introduced into the shared memory.
>
> Now:
>
> - All the processes but the library one that reports the results exit
>   with 0
> - tst_brk(TBROK, ...) increments result conters, sets the abort flag.
>   and exit current process
> - all other tst_brk() variants will just increments the countes and
>   exits the current process

It removes the easy way for parent to check that child hasn't run into
any issues,
but I can't recall a specific test we have today that depends on it.

>
> This makes the tst_brk() behavior well defined so we can now even call
> tst_brk() with TFAIL and TPASS as well.

What's the use-case for it? Wouldn't it be more clear to just report
TPASS + exit?

>
> Open question (may be done in a separatep patch):
>
> Should tst_brk(TBROK, ...) apart from setting the flag also send sigkill
> signal to the test process group to kill any leftover test processes?

Or heartbeat checking the abort flag and doing it from the library?

>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  include/tst_test.h          | 24 +++++++-------
>  lib/newlib_tests/.gitignore |  6 +++-
>  lib/newlib_tests/test23.c   | 25 ++++++++++++++
>  lib/newlib_tests/test24.c   | 25 ++++++++++++++
>  lib/newlib_tests/test25.c   | 19 +++++++++++
>  lib/newlib_tests/test26.c   | 20 +++++++++++
>  lib/tst_test.c              | 66 +++++++++++++++++++++----------------
>  7 files changed, 144 insertions(+), 41 deletions(-)
>  create mode 100644 lib/newlib_tests/test23.c
>  create mode 100644 lib/newlib_tests/test24.c
>  create mode 100644 lib/newlib_tests/test25.c
>  create mode 100644 lib/newlib_tests/test26.c
>
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 8d1819f74..7ad91e9fa 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -98,28 +98,30 @@ void tst_brk_(const char *file, const int lineno, int ttype,
>                __attribute__ ((format (printf, 4, 5)));
>
>  /**
> - * tst_brk() - Reports a breakage and exits the test.
> + * tst_brk() - Reports a breakage and exits the test or test process.
>   *
>   * @ttype: An enum tst_res_type.
>   * @arg_fmt: A printf-like format.
>   * @...: A printf-like parameters.
>   *
> - * Reports either TBROK or TCONF and exits the test immediately. When called
> - * all children in the same process group as the main test library process are
> - * killed. This function, unless in a test cleanup, calls _exit() and does not
> - * return.
> + * Reports a single result and exits immediately. The call behaves differently
> + * based on the ttype parameter. For all ttype results but TBROK the call exits
> + * the current test process, i.e. increments test result counters and calls
> + * exit(0).
> + *
> + * The TBROK ttype is special that apart from exitting the current test process
> + * it also tells to the test library to exit immediatelly. This means that any
> + * subsequent test iterations are not executed, e.g. if tests runs for all
> + * filesystems but tst_brk() with TBROK is called, the test exits and does not
> + * attempt to continue a test iteration for the next filtesystem.
>   *
>   * When test is in cleanup() function TBROK is converted into TWARN by the test
>   * library and we attempt to carry on with a cleanup even when tst_brk() was
>   * called. This makes it possible to use SAFE_FOO() macros in the test cleanup
>   * without interrupting the cleanup process on a failure.
>   */
> -#define tst_brk(ttype, arg_fmt, ...)                                           \
> -       ({                                                                      \
> -               TST_BRK_SUPPORTS_ONLY_TCONF_TBROK(!((ttype) &                   \
> -                       (TBROK | TCONF | TFAIL)));                              \
> -               tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
> -       })
> +#define tst_brk(ttype, arg_fmt, ...) \
> +               tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__)
>
>  void tst_printf(const char *const fmt, ...)
>                 __attribute__((nonnull(1), format (printf, 1, 2)));
> diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
> index 6d125f933..b3b662d51 100644
> --- a/lib/newlib_tests/.gitignore
> +++ b/lib/newlib_tests/.gitignore
> @@ -23,6 +23,10 @@ tst_print_result
>  test19
>  test20
>  test22
> +test23
> +test24
> +test25
> +test26
>  tst_expiration_timer
>  test_assert
>  test_timer
> @@ -58,4 +62,4 @@ test_runtime01
>  test_runtime02
>  test_children_cleanup
>  tst_res_flags
> -tst_safe_sscanf
> \ No newline at end of file
> +tst_safe_sscanf
> diff --git a/lib/newlib_tests/test23.c b/lib/newlib_tests/test23.c
> new file mode 100644
> index 000000000..f3ec29793
> --- /dev/null
> +++ b/lib/newlib_tests/test23.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2024 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +/*
> + * Test that tst_brk(TFAIL, ...) works properly.
> + */
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +       int pid = SAFE_FORK();
> +
> +       if (pid)
> +               return;
> +
> +       tst_brk(TFAIL, "Test child exitting...");
> +       tst_res(TWARN, "Test child stil alive!");
> +}
> +
> +static struct tst_test test = {
> +       .test_all = do_test,
> +       .forks_child = 1,
> +};
> diff --git a/lib/newlib_tests/test24.c b/lib/newlib_tests/test24.c
> new file mode 100644
> index 000000000..423383cfd
> --- /dev/null
> +++ b/lib/newlib_tests/test24.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2024 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +/*
> + * Test that tst_brk(TPASS, ...) works properly.
> + */
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +       int pid = SAFE_FORK();
> +
> +       if (pid)
> +               return;
> +
> +       tst_brk(TPASS, "Test child exitting...");
> +       tst_res(TWARN, "Test child stil alive!");
> +}
> +
> +static struct tst_test test = {
> +       .test_all = do_test,
> +       .forks_child = 1,
> +};
> diff --git a/lib/newlib_tests/test25.c b/lib/newlib_tests/test25.c
> new file mode 100644
> index 000000000..0795ca7d0
> --- /dev/null
> +++ b/lib/newlib_tests/test25.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2024 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +/*
> + * Test that tst_brk(TBROK, ...) exits the test immediatelly.
> + */
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +       tst_brk(TBROK, "Exitting the test during the first variant");
> +}
> +
> +static struct tst_test test = {
> +       .test_all = do_test,
> +       .test_variants = 10,
> +};
> diff --git a/lib/newlib_tests/test26.c b/lib/newlib_tests/test26.c
> new file mode 100644
> index 000000000..27829c703
> --- /dev/null
> +++ b/lib/newlib_tests/test26.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2024 Cyril Hrubis <chrubis@suse.cz>
> + */
> +
> +/*
> + * Test that tst_brk(TFAIL, ...) exits only single test variant.
> + */
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +       tst_brk(TFAIL, "Failing a test variant");
> +       tst_res(TWARN, "Shouldn't be reached");
> +}
> +
> +static struct tst_test test = {
> +       .test_all = do_test,
> +       .test_variants = 10,
> +};
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 8db554dea..9dd00718a 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -71,6 +71,13 @@ struct results {
>         int failed;
>         int warnings;
>         int broken;
> +
> +       /*
> +        * This is set by a call to tst_brk() with TBROK parameter and means
> +        * that the test should exit immediatelly.
> +        */
> +       int abort_flag;
> +
>         unsigned int timeout;
>         int max_runtime;
>  };
> @@ -395,7 +402,16 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
>         if (getpid() == lib_pid)
>                 do_exit(TTYPE_RESULT(ttype));
>
> -       exit(TTYPE_RESULT(ttype));
> +       /*
> +        * If we get here we are in a child process, either the main child
> +        * running the test or its children. If any of them called tst_brk()
> +        * with TBROK we need to exit the test. Otherwise we just exit the
> +        * current process.
> +        */
> +       if (TTYPE_RESULT(ttype) == TBROK)
> +               tst_atomic_inc(&results->abort_flag);
> +
> +       exit(0);
>  }
>
>  void tst_res_(const char *file, const int lineno, int ttype,
> @@ -432,8 +448,6 @@ void tst_printf(const char *const fmt, ...)
>
>  static void check_child_status(pid_t pid, int status)
>  {
> -       int ret;
> -
>         if (WIFSIGNALED(status)) {
>                 tst_brk(TBROK, "Child (%i) killed by signal %s", pid,
>                         tst_strsig(WTERMSIG(status)));
> @@ -442,15 +456,8 @@ static void check_child_status(pid_t pid, int status)
>         if (!(WIFEXITED(status)))
>                 tst_brk(TBROK, "Child (%i) exited abnormally", pid);
>
> -       ret = WEXITSTATUS(status);
> -       switch (ret) {
> -       case TPASS:
> -       case TBROK:
> -       case TCONF:
> -       break;
> -       default:
> -               tst_brk(TBROK, "Invalid child (%i) exit value %i", pid, ret);
> -       }
> +       if (WEXITSTATUS(status))
> +               tst_brk(TBROK, "Invalid child (%i) exit value %i", pid, WEXITSTATUS(status));
>  }
>
>  void tst_reap_children(void)
> @@ -915,6 +922,14 @@ static void print_failure_hints(void)
>         show_failure_hints = 0;
>  }
>
> +/*
> + * Prints results, cleans up after the test library and exits the test library
> + * process. The ret parameter is used to pass the result flags in a case of a
> + * failure before we managed to set up the shared memory where we store the
> + * results. This allows us to use SAFE_MACROS() in the initialization of the
> + * shared memory. The ret parameter is not used (passed 0) when called
> + * explicitly from the rest of the library.
> + */
>  static void do_exit(int ret)
>  {
>         if (results) {
> @@ -1555,6 +1570,7 @@ static void run_tests(void)
>
>                 if (results_equal(&saved_results, results))
>                         tst_brk(TBROK, "Test haven't reported results!");
> +
>                 return;
>         }
>
> @@ -1775,7 +1791,7 @@ static int fork_testrun(void)
>                 tst_res(TINFO, "Killed the leftover descendant processes");
>
>         if (WIFEXITED(status) && WEXITSTATUS(status))
> -               return WEXITSTATUS(status);
> +               tst_brk(TBROK, "Child returned with %i", WEXITSTATUS(status));
>
>         if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL) {
>                 tst_res(TINFO, "If you are running on slow machine, "
> @@ -1856,15 +1872,10 @@ static int run_tcases_per_fs(void)
>                 if (!fs)
>                         continue;
>
> -               ret = run_tcase_on_fs(fs, filesystems[i]);
> +               run_tcase_on_fs(fs, filesystems[i]);
>
> -               if (ret == TCONF)
> -                       continue;
> -
> -               if (ret == 0)
> -                       continue;
> -
> -               do_exit(ret);
> +               if (tst_atomic_load(&results->abort_flag))
> +                       do_exit(0);
>         }
>
>         return ret;
> @@ -1874,7 +1885,6 @@ unsigned int tst_variant;
>
>  void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
>  {
> -       int ret = 0;
>         unsigned int test_variants = 1;
>         struct utsname uval;
>
> @@ -1903,19 +1913,17 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
>
>         for (tst_variant = 0; tst_variant < test_variants; tst_variant++) {
>                 if (tst_test->all_filesystems || count_fs_descs() > 1)
> -                       ret |= run_tcases_per_fs();
> +                       run_tcases_per_fs();
>                 else
> -                       ret |= fork_testrun();
> +                       fork_testrun();
>
> -               if (ret & ~(TCONF))
> -                       goto exit;
> +               if (tst_atomic_load(&results->abort_flag))
> +                       do_exit(0);
>         }
>
> -exit:
> -       do_exit(ret);
> +       do_exit(0);
>  }
>
> -
>  void tst_flush(void)
>  {
>         int rval;
> --
> 2.45.2
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>

Hi!
> > This makes the tst_brk() handling cleaner and saner as instead of
> > propagating the tst_brk() result in a return value an abort flag is
> > introduced into the shared memory.
> >
> > Now:
> >
> > - All the processes but the library one that reports the results exit
> >   with 0
> > - tst_brk(TBROK, ...) increments result conters, sets the abort flag.
> >   and exit current process
> > - all other tst_brk() variants will just increments the countes and
> >   exits the current process
> 
> It removes the easy way for parent to check that child hasn't run into
> any issues,
> but I can't recall a specific test we have today that depends on it.

I suppose that we can make the tst_brk flag part of a public API if
anyone needs that but I guess that in the case of tst_brk(TBROK, ...)
all we want is to make the test processes exit as soon as possible.

> > This makes the tst_brk() behavior well defined so we can now even call
> > tst_brk() with TFAIL and TPASS as well.
> 
> What's the use-case for it? Wouldn't it be more clear to just report
> TPASS + exit?

I think this makes actually the API more consistent. I.e. tst_res()
reports result and tst_brk() reports result and exits the current
process.

I think that we all carry a mental baggage that associates the tst_brk()
call with an error, but that is something we forced upon ourselves.
AFAIK it's short for tst_break, which itself only suggests that it does
exit the current process, similar to break being used in switch()
statement.

> > Open question (may be done in a separatep patch):
> >
> > Should tst_brk(TBROK, ...) apart from setting the flag also send sigkill
> > signal to the test process group to kill any leftover test processes?
> 
> Or heartbeat checking the abort flag and doing it from the library?

The heartbeat handler may be a good place to put this check into and I
was also thinking of adding checks to all SAFE_MACROS() and make them
something as cancelation points since they are supposed to exit the test
on a failure anyways.

But that could be only done once we have the flag in place and finally
have persistent way how to check that something went wrong.

On Tue, Nov 26, 2024 at 10:50 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > This makes the tst_brk() handling cleaner and saner as instead of
> > > propagating the tst_brk() result in a return value an abort flag is
> > > introduced into the shared memory.
> > >
> > > Now:
> > >
> > > - All the processes but the library one that reports the results exit
> > >   with 0
> > > - tst_brk(TBROK, ...) increments result conters, sets the abort flag.
> > >   and exit current process
> > > - all other tst_brk() variants will just increments the countes and
> > >   exits the current process
> >
> > It removes the easy way for parent to check that child hasn't run into
> > any issues,
> > but I can't recall a specific test we have today that depends on it.
>
> I suppose that we can make the tst_brk flag part of a public API if
> anyone needs that but I guess that in the case of tst_brk(TBROK, ...)
> all we want is to make the test processes exit as soon as possible.
>
> > > This makes the tst_brk() behavior well defined so we can now even call
> > > tst_brk() with TFAIL and TPASS as well.
> >
> > What's the use-case for it? Wouldn't it be more clear to just report
> > TPASS + exit?
>
> I think this makes actually the API more consistent. I.e. tst_res()
> reports result and tst_brk() reports result and exits the current
> process.
>
> I think that we all carry a mental baggage that associates the tst_brk()
> call with an error, but that is something we forced upon ourselves.
> AFAIK it's short for tst_break, which itself only suggests that it does
> exit the current process, similar to break being used in switch()
> statement.
>

Indeed. With this change, we don't have to use "tst_res() + return" for
some situations,
only tst_brk(TPASS, ) would be enough.

Hi all,

new approach looks as an improvement to me.

So you plan to send v2 to send sigkill signal to the test process group in
the heartbeat handler?

> On Tue, Nov 26, 2024 at 10:50 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> > Hi!
> > > > This makes the tst_brk() handling cleaner and saner as instead of
> > > > propagating the tst_brk() result in a return value an abort flag is
> > > > introduced into the shared memory.

> > > > Now:

> > > > - All the processes but the library one that reports the results exit
> > > >   with 0
> > > > - tst_brk(TBROK, ...) increments result conters, sets the abort flag.
> > > >   and exit current process
> > > > - all other tst_brk() variants will just increments the countes and
> > > >   exits the current process

> > > It removes the easy way for parent to check that child hasn't run into
> > > any issues,
> > > but I can't recall a specific test we have today that depends on it.

> > I suppose that we can make the tst_brk flag part of a public API if
> > anyone needs that but I guess that in the case of tst_brk(TBROK, ...)
> > all we want is to make the test processes exit as soon as possible.

> > > > This makes the tst_brk() behavior well defined so we can now even call
> > > > tst_brk() with TFAIL and TPASS as well.

> > > What's the use-case for it? Wouldn't it be more clear to just report
> > > TPASS + exit?

> > I think this makes actually the API more consistent. I.e. tst_res()
> > reports result and tst_brk() reports result and exits the current
> > process.

+1

> > I think that we all carry a mental baggage that associates the tst_brk()
> > call with an error, but that is something we forced upon ourselves.
> > AFAIK it's short for tst_break, which itself only suggests that it does
> > exit the current process, similar to break being used in switch()
> > statement.

+1.

> Indeed. With this change, we don't have to use "tst_res() + return" for
> some situations,
> only tst_brk(TPASS, ) would be enough.

+1. We have tst_brk(TFAIL) anyway. We should also unify shell API.

nit (commenting the implementation):

+/*
Don't we want to have it in the docs, e.g. /** + document parameters?
+ * Prints results, cleans up after the test library and exits the test library

+++ b/lib/newlib_tests/test23.c
very nit: I would prefer short description names for tests than numbers.

Kind regards,
Petr