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

[LTP] [PATCH] 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.

And since TBROK is supposed to exit the test immediatelly (i.e.
unrecoverable error) we are now properly doing so.

The case that main test pid called TBROK was working correctly before
this patch, since send the SIGKILL signal to he process group after we
waited for the main test pid. All that was missing is a code that sends
a signal to the main test pid in the case that TBROK was triggered by
one of it's children and now we properly kill all test processes in that
case as well.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 include/tst_test.h                   | 26 +++++----
 lib/newlib_tests/.gitignore          |  8 ++-
 lib/newlib_tests/test_brk_child.c    | 31 +++++++++++
 lib/newlib_tests/test_brk_fail.c     | 25 +++++++++
 lib/newlib_tests/test_brk_parent.c   | 28 ++++++++++
 lib/newlib_tests/test_brk_pass.c     | 25 +++++++++
 lib/newlib_tests/test_brk_variant.c  | 19 +++++++
 lib/newlib_tests/test_fail_variant.c | 20 +++++++
 lib/tst_test.c                       | 81 +++++++++++++++++-----------
 9 files changed, 221 insertions(+), 42 deletions(-)
 create mode 100644 lib/newlib_tests/test_brk_child.c
 create mode 100644 lib/newlib_tests/test_brk_fail.c
 create mode 100644 lib/newlib_tests/test_brk_parent.c
 create mode 100644 lib/newlib_tests/test_brk_pass.c
 create mode 100644 lib/newlib_tests/test_brk_variant.c
 create mode 100644 lib/newlib_tests/test_fail_variant.c

diff --git a/include/tst_test.h b/include/tst_test.h
index eb73cd593..64b034b36 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -98,28 +98,32 @@ 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. When TBROK is
+ * triggered by any of the test processes the whole process group is killed so
+ * that there are no processes left after the library process exits. This also
+ * means that any subsequent test iterations are not executed, e.g. if a test
+ * runs for all filesystems and 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..8acaec0b6 100644
--- a/lib/newlib_tests/.gitignore
+++ b/lib/newlib_tests/.gitignore
@@ -58,4 +58,10 @@ test_runtime01
 test_runtime02
 test_children_cleanup
 tst_res_flags
-tst_safe_sscanf
\ No newline at end of file
+tst_safe_sscanf
+test_brk_child
+test_brk_fail
+test_brk_parent
+test_brk_pass
+test_brk_variant
+test_fail_variant
diff --git a/lib/newlib_tests/test_brk_child.c b/lib/newlib_tests/test_brk_child.c
new file mode 100644
index 000000000..68c50ed1e
--- /dev/null
+++ b/lib/newlib_tests/test_brk_child.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2025 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+/*
+ * Test that tst_brk(TFAIL, ...) exits only single test variant.
+ */
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	int i;
+
+	for (i = 0; i < 10; i++) {
+		if (!SAFE_FORK()) {
+			tst_res(TINFO, "Suspending child %i", i);
+			pause();
+		}
+	}
+
+	if (!SAFE_FORK())
+		tst_brk(TBROK, "Child triggers TBROK");
+
+	pause();
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.forks_child = 1,
+};
diff --git a/lib/newlib_tests/test_brk_fail.c b/lib/newlib_tests/test_brk_fail.c
new file mode 100644
index 000000000..f3ec29793
--- /dev/null
+++ b/lib/newlib_tests/test_brk_fail.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/test_brk_parent.c b/lib/newlib_tests/test_brk_parent.c
new file mode 100644
index 000000000..974e77829
--- /dev/null
+++ b/lib/newlib_tests/test_brk_parent.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2025 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+/*
+ * Test that tst_brk(TFAIL, ...) exits only single test variant.
+ */
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	int i;
+
+	for (i = 0; i < 10; i++) {
+		if (!SAFE_FORK()) {
+			tst_res(TINFO, "Suspending child %i", i);
+			pause();
+		}
+	}
+
+	tst_brk(TBROK, "Parent triggers TBROK");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.forks_child = 1,
+};
diff --git a/lib/newlib_tests/test_brk_pass.c b/lib/newlib_tests/test_brk_pass.c
new file mode 100644
index 000000000..423383cfd
--- /dev/null
+++ b/lib/newlib_tests/test_brk_pass.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/test_brk_variant.c b/lib/newlib_tests/test_brk_variant.c
new file mode 100644
index 000000000..0795ca7d0
--- /dev/null
+++ b/lib/newlib_tests/test_brk_variant.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/test_fail_variant.c b/lib/newlib_tests/test_fail_variant.c
new file mode 100644
index 000000000..27829c703
--- /dev/null
+++ b/lib/newlib_tests/test_fail_variant.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 9a23cd4a0..c6395a5eb 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Copyright (c) 2015-2016 Cyril Hrubis <chrubis@suse.cz>
+ * Copyright (c) 2015-2025 Cyril Hrubis <chrubis@suse.cz>
  * Copyright (c) Linux Test Project, 2016-2024
  */
 
@@ -71,6 +71,11 @@ 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 runtime;
 	unsigned int overall_time;
 };
@@ -395,7 +400,27 @@ 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);
+
+		/*
+		 * If TBROK was called from one of the child processes we kill
+		 * the main test process. That in turn triggers the code that
+		 * kills leftover children once the main test process did exit.
+		 */
+		if (tst_getpid() != main_pid) {
+			tst_res(TINFO, "Child process reported TBROK killing the test");
+			kill(main_pid, SIGKILL);
+		}
+	}
+
+	exit(0);
 }
 
 void tst_res_(const char *file, const int lineno, int ttype,
@@ -432,8 +457,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 +465,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)
@@ -912,6 +928,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) {
@@ -1557,6 +1581,7 @@ static void run_tests(void)
 
 		if (results_equal(&saved_results, results))
 			tst_brk(TBROK, "Test haven't reported results!");
+
 		return;
 	}
 
@@ -1651,6 +1676,7 @@ static volatile sig_atomic_t sigkill_retries;
 static void alarm_handler(int sig LTP_ATTRIBUTE_UNUSED)
 {
 	WRITE_MSG("Test timeouted, sending SIGKILL!\n");
+
 	kill(-test_pid, SIGKILL);
 	alarm(5);
 
@@ -1785,7 +1811,10 @@ 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 (results->abort_flag)
+		return 0;
 
 	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL) {
 		tst_res(TINFO, "If you are running on slow machine, "
@@ -1868,15 +1897,10 @@ static int run_tcases_per_fs(void)
 			continue;
 
 		found_valid_fs = true;
-		ret = run_tcase_on_fs(fs, filesystems[i]);
-
-		if (ret == TCONF)
-			continue;
+		run_tcase_on_fs(fs, filesystems[i]);
 
-		if (ret == 0)
-			continue;
-
-		do_exit(ret);
+		if (tst_atomic_load(&results->abort_flag))
+			do_exit(0);
 	}
 
 	if (!found_valid_fs)
@@ -1889,7 +1913,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;
 
@@ -1918,19 +1941,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.49.0


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

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

[Petr Vorel]

Hi all,

[ Cc Jan and Li s there can be some hidden problems. ]

> 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.

+1. As a separate effort we can replace with coccinelle
tst_res(TFAIL or TPASS) followed by return by tst_brk(TFAIL or TPASS).

> And since TBROK is supposed to exit the test immediatelly (i.e.
> unrecoverable error) we are now properly doing so.

> The case that main test pid called TBROK was working correctly before
> this patch, since send the SIGKILL signal to he process group after we
> waited for the main test pid. All that was missing is a code that sends
> a signal to the main test pid in the case that TBROK was triggered by
> one of it's children and now we properly kill all test processes in that
> case as well.

+1

Also thanks for doing tests.

Generally LGTM.

One thing I'm worried is the fact that some shell loader tests core dumped in CI:
https://github.com/pevik/ltp/actions/runs/14242818586/job/39916477770
e.g. these which are supposed to TBROK due broken metadata:
shell_loader_invalid_block.sh, shell_loader_no_metadata.sh,
shell_loader_wrong_metadata.sh:

Segmentation fault (core dumped)

I also wonder if tst_brk() related doc should be updated.

Isn't there anything which should be updated in doc/old/C-Test-API.asciidoc ?
This docs will stay with us for some time, the conversion to kerneldoc takes
time, it'd be good to keep it updated (valuable texts will be migrated to
kerneldoc).

Maybe parts:
1.8 Doing the test in the child process
1.9 Fork() and Parent-child synchronization
(both code examples and the description).

very nit: please before merge fix typos in both code and commit message:
exitting => exiting
countes|countes => counters
immediatelly => immediately
filtesystem => filesystem

NOTE: test_brk_pass could be added to lib/newlib_tests/runtest.sh. I would also
prefer, if we changed tests to behave like testcases/lib/run_tests.sh, i.e.
allow to run all tests and check exit code (intermediate step before we compare
the test output).

Kind regards,
Petr

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

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

[Cyril Hrubis]

Hi!
> One thing I'm worried is the fact that some shell loader tests core dumped in CI:
> https://github.com/pevik/ltp/actions/runs/14242818586/job/39916477770
> e.g. these which are supposed to TBROK due broken metadata:
> shell_loader_invalid_block.sh, shell_loader_no_metadata.sh,
> shell_loader_wrong_metadata.sh:
> 
> Segmentation fault (core dumped)

Ah, that's because if we call tst_brk() before we finished
initialization results is not defined. We need at least:

diff --git a/lib/tst_test.c b/lib/tst_test.c
index c6395a5eb..6b1100b09 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -407,7 +407,8 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
         * current process.
         */
        if (TTYPE_RESULT(ttype) == TBROK) {
-               tst_atomic_inc(&results->abort_flag);
+               if (results)
+                       tst_atomic_inc(&results->abort_flag);

                /*
                 * If TBROK was called from one of the child processes we kill

> I also wonder if tst_brk() related doc should be updated.

I did update the documentation comment in the tst_test.h.

> Isn't there anything which should be updated in doc/old/C-Test-API.asciidoc ?
> This docs will stay with us for some time, the conversion to kerneldoc takes
> time, it'd be good to keep it updated (valuable texts will be migrated to
> kerneldoc).
> 
> Maybe parts:
> 1.8 Doing the test in the child process
> 1.9 Fork() and Parent-child synchronization
> (both code examples and the description).

I will have a look.

> very nit: please before merge fix typos in both code and commit message:
> exitting => exiting
> countes|countes => counters
> immediatelly => immediately
> filtesystem => filesyste

Uff, will do.

> NOTE: test_brk_pass could be added to lib/newlib_tests/runtest.sh. I would also
> prefer, if we changed tests to behave like testcases/lib/run_tests.sh, i.e.
> allow to run all tests and check exit code (intermediate step before we compare
> the test output).

Rewriting the tests is a more complex task that should be done in a
separate patchset.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Cyril Hrubis]

Hi!
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index c6395a5eb..6b1100b09 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -407,7 +407,8 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
>          * current process.
>          */
>         if (TTYPE_RESULT(ttype) == TBROK) {
> -               tst_atomic_inc(&results->abort_flag);
> +               if (results)
> +                       tst_atomic_inc(&results->abort_flag);
> 
>                 /*
>                  * If TBROK was called from one of the child processes we kill
> 

And it's a bit more complex we also need to make sure to exit properly
when tst_brk() was called before the library was initialized, so we also
need:

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 758e62823..d19908d94 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -386,6 +386,14 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
               va_list va)
 {
        print_result(file, lineno, ttype, fmt, va);
+
+       /*
+        * If tst_brk() is called from some of the C helpers even before the
+        * library was initialized, just exit.
+        */
+       if (!lib_pid)
+               exit(TTYPE_RESULT(ttype));
+
        update_results(TTYPE_RESULT(ttype));

        /*
@@ -415,7 +423,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
                 * the main test process. That in turn triggers the code that
                 * kills leftover children once the main test process did exit.
                 */
-               if (tst_getpid() != main_pid) {
+               if (main_pid && tst_getpid() != main_pid) {
                        tst_res(TINFO, "Child process reported TBROK killing the test");
                        kill(main_pid, SIGKILL);
                }


I will send v2 later on.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Petr Vorel]

> Hi!
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index c6395a5eb..6b1100b09 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -407,7 +407,8 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
> >          * current process.
> >          */
> >         if (TTYPE_RESULT(ttype) == TBROK) {
> > -               tst_atomic_inc(&results->abort_flag);
> > +               if (results)
> > +                       tst_atomic_inc(&results->abort_flag);

> >                 /*
> >                  * If TBROK was called from one of the child processes we kill


> And it's a bit more complex we also need to make sure to exit properly
> when tst_brk() was called before the library was initialized, so we also

+1

> need:

> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 758e62823..d19908d94 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -386,6 +386,14 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
>                va_list va)
>  {
>         print_result(file, lineno, ttype, fmt, va);
> +
> +       /*
> +        * If tst_brk() is called from some of the C helpers even before the
> +        * library was initialized, just exit.
> +        */
> +       if (!lib_pid)
> +               exit(TTYPE_RESULT(ttype));

Interesting, I never noticed lib_pid :). It's assigned in tst_run_tcases() -
library gets initialized there. As that's the very first call in main(), this
really applies for helpers in testcases/lib/ which define TST_NO_DEFAULT_MAIN.

I suppose this is the part of the fix for core dumped helpers.

> +
>         update_results(TTYPE_RESULT(ttype));

>         /*
> @@ -415,7 +423,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
>                  * the main test process. That in turn triggers the code that
>                  * kills leftover children once the main test process did exit.
>                  */
> -               if (tst_getpid() != main_pid) {
> +               if (main_pid && tst_getpid() != main_pid) {
>                         tst_res(TINFO, "Child process reported TBROK killing the test");
>                         kill(main_pid, SIGKILL);
>                 }


> I will send v2 later on.

+1

Kind regards,
Petr

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

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

[Jan Stancek via ltp]

On Fri, Apr 4, 2025 at 2:55 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > Hi!
> > > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > > index c6395a5eb..6b1100b09 100644
> > > --- a/lib/tst_test.c
> > > +++ b/lib/tst_test.c
> > > @@ -407,7 +407,8 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
> > >          * current process.
> > >          */
> > >         if (TTYPE_RESULT(ttype) == TBROK) {
> > > -               tst_atomic_inc(&results->abort_flag);
> > > +               if (results)
> > > +                       tst_atomic_inc(&results->abort_flag);
>
> > >                 /*
> > >                  * If TBROK was called from one of the child processes we kill
>
>
> > And it's a bit more complex we also need to make sure to exit properly
> > when tst_brk() was called before the library was initialized, so we also
>
> +1
>
> > need:
>
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index 758e62823..d19908d94 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -386,6 +386,14 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
> >                va_list va)
> >  {
> >         print_result(file, lineno, ttype, fmt, va);
> > +
> > +       /*
> > +        * If tst_brk() is called from some of the C helpers even before the
> > +        * library was initialized, just exit.
> > +        */
> > +       if (!lib_pid)
> > +               exit(TTYPE_RESULT(ttype));
>
> Interesting, I never noticed lib_pid :). It's assigned in tst_run_tcases() -
> library gets initialized there. As that's the very first call in main(), this
> really applies for helpers in testcases/lib/ which define TST_NO_DEFAULT_MAIN.
>
> I suppose this is the part of the fix for core dumped helpers.
>
> > +
> >         update_results(TTYPE_RESULT(ttype));
>
> >         /*
> > @@ -415,7 +423,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
> >                  * the main test process. That in turn triggers the code that
> >                  * kills leftover children once the main test process did exit.
> >                  */
> > -               if (tst_getpid() != main_pid) {
> > +               if (main_pid && tst_getpid() != main_pid) {
> >                         tst_res(TINFO, "Child process reported TBROK killing the test");
> >                         kill(main_pid, SIGKILL);
> >                 }
>
>
> > I will send v2 later on.
>
> +1

+1, I wanted to have a look too, and it will be easier to follow
having it all in together.

>
> Kind regards,
> Petr
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>


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

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

[Petr Vorel]

Hi Cyril,

> Hi!
> > One thing I'm worried is the fact that some shell loader tests core dumped in CI:
> > https://github.com/pevik/ltp/actions/runs/14242818586/job/39916477770
> > e.g. these which are supposed to TBROK due broken metadata:
> > shell_loader_invalid_block.sh, shell_loader_no_metadata.sh,
> > shell_loader_wrong_metadata.sh:

> > Segmentation fault (core dumped)

> Ah, that's because if we call tst_brk() before we finished
> initialization results is not defined. We need at least:

> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index c6395a5eb..6b1100b09 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -407,7 +407,8 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
>          * current process.
>          */
>         if (TTYPE_RESULT(ttype) == TBROK) {
> -               tst_atomic_inc(&results->abort_flag);
> +               if (results)
> +                       tst_atomic_inc(&results->abort_flag);

>                 /*
>                  * If TBROK was called from one of the child processes we kill

> > I also wonder if tst_brk() related doc should be updated.

> I did update the documentation comment in the tst_test.h.

I'm sorry to overlook this. The description LGTM.

+ * runs for all filesystems and tst_brk() with TBROK is called, the test exits
+ * and does not attempt to continue a test iteration for the next filesystem.

I guess this part of the doc update is also applicable for the current master,
right? Quite important note, indeed TCONF does not quit testing on
all_filesystems.  Probably not worth to have it as a separate commit (hopefully
this change will not get reverted).

> > Isn't there anything which should be updated in doc/old/C-Test-API.asciidoc ?
> > This docs will stay with us for some time, the conversion to kerneldoc takes
> > time, it'd be good to keep it updated (valuable texts will be migrated to
> > kerneldoc).

> > Maybe parts:
> > 1.8 Doing the test in the child process
> > 1.9 Fork() and Parent-child synchronization
> > (both code examples and the description).

> I will have a look.

Thanks!

> > very nit: please before merge fix typos in both code and commit message:
> > exitting => exiting
> > countes|countes => counters
> > immediatelly => immediately
> > filtesystem => filesyste

> Uff, will do.

Thanks. (I have spell checker on patches, thus I see it red.)

> > NOTE: test_brk_pass could be added to lib/newlib_tests/runtest.sh. I would also
> > prefer, if we changed tests to behave like testcases/lib/run_tests.sh, i.e.
> > allow to run all tests and check exit code (intermediate step before we compare
> > the test output).

> Rewriting the tests is a more complex task that should be done in a
> separate patchset.

Sure, this was more sharing my idea (I planned to write "as a separate effort",
I see I forget).

Kind regards,
Petr

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

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

[Cyril Hrubis]

Hi!
> Interesting, I never noticed lib_pid :). It's assigned in tst_run_tcases() -
> library gets initialized there. As that's the very first call in main(), this
> really applies for helpers in testcases/lib/ which define TST_NO_DEFAULT_MAIN.
> 
> I suppose this is the part of the fix for core dumped helpers.

The tst_run_shell.c needs to fill in the tst_test structure first and
that is done before the test library was initialized. If we happen to
call tst_brk() in that phase we have to make sure that we do not touch
the results structure, which wasn't mapped yet and that we exit() with
right exit value. After these changes, once the test library has been
initialized all processes but the test library has to exit with success
and the final exit value is produce by the test library process.

-- 
Cyril Hrubis
chrubis@suse.cz

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