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

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

[Cyril Hrubis]

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

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

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

[Jan Stancek]

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
>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Li Wang]

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.


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Petr Vorel]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp