[LTP] [PATCH] [RFC] lib/tst_test.c: Fix tst_brk() handling
Jan Stancek
jstancek@redhat.com
Mon Nov 18 11:39:14 CET 2024
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
>
More information about the ltp
mailing list