[LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[Petr Vorel]

This helps readability on tests with high tcnt.

Inspired by tst_test.sh (shell API) which prints $TST_COUNT in tst_res
and tst_brk.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
NOTE: testing on open01.c which has tcnt = 2.
Before:
tst_tmpdir.c:308: TINFO: Using /tmp/LTP_opebwazQS as tmpdir (tmpfs filesystem)
tst_test.c:2059: TINFO: LTP version: 20260130-78-g68660b12c8
tst_test.c:1887: TINFO: Overall timeout per run is 0h 00m 30s
open01.c:56: TPASS: sticky bit is set as expected
open01.c:56: TPASS: directory bit is set as expected

With the patch:

tst_tmpdir.c:308: 1 TINFO: Using /tmp/LTP_opezEOyj2 as tmpdir (tmpfs filesystem)
tst_test.c:2059: 1 TINFO: LTP version: 20260130-79-g8d8379aa73
tst_test.c:1887: 1 TINFO: Overall timeout per run is 0h 00m 30s
open01.c:56: 1 TPASS: sticky bit is set as expected
open01.c:56: 2 TPASS: directory bit is set as expected

Setup has "1", cleanup has tcnt + 1. We could even print "0" for setup
or even for cleanup as well.

Kind regards,
Petr

 lib/tst_test.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 2d62ab164d..b9593b743a 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -69,6 +69,7 @@ static float duration = -1;
 static float timeout_mul = -1;
 static int reproducible_output;
 static int quiet_output;
+static unsigned int tcnt;
 
 struct context {
 	int32_t lib_pid;
@@ -344,7 +345,7 @@ static void print_result(const char *file, const int lineno, int ttype,
 		str_errno = tst_strerrno(int_errno);
 	}
 
-	ret = snprintf(str, size, "%s:%i: ", file, lineno);
+	ret = snprintf(str, size, "%s:%i: %u ", file, lineno, tcnt+1);
 	str += ret;
 	size -= ret;
 
@@ -1705,7 +1706,6 @@ static void heartbeat(void)
 
 static void run_tests(void)
 {
-	unsigned int i;
 	struct results saved_results;
 
 	if (!tst_test->test) {
@@ -1724,10 +1724,10 @@ static void run_tests(void)
 		return;
 	}
 
-	for (i = 0; i < tst_test->tcnt; i++) {
+	for (tcnt = 0; tcnt < tst_test->tcnt; tcnt++) {
 		saved_results = *results;
 		heartbeat();
-		tst_test->test(i);
+		tst_test->test(tcnt);
 
 		if (tst_getpid() != context->main_pid)
 			exit(0);
@@ -1735,7 +1735,7 @@ static void run_tests(void)
 		tst_reap_children();
 
 		if (results_equal(&saved_results, results))
-			tst_brk(TBROK, "Test %i haven't reported results!", i);
+			tst_brk(TBROK, "Test %i haven't reported results!", tcnt);
 	}
 }
 
-- 
2.51.0


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

Re: [LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[Andrea Cervesato via ltp]

Hi Petr,

the idea is ok, but there are a few issues in the implementation.

> +static unsigned int tcnt;

`tcnt` is misleading because we are already using the same name inside
`struct tst_test`. Better to use `tcase_idx` or something like this.

>  
>  struct context {
>  	int32_t lib_pid;
> @@ -344,7 +345,7 @@ static void print_result(const char *file, const int lineno, int ttype,

print_result() is used everywhere, by any tst_brk, tst_res, tst_vbrk_, tst_cvres
etc. Which means you will have a `tcnt` printed everywhere. It looks better if
we select only the starting message.

>  
>  		if (results_equal(&saved_results, results))
> -			tst_brk(TBROK, "Test %i haven't reported results!", i);
> +			tst_brk(TBROK, "Test %i haven't reported results!", tcnt);

The new prefix printed by print_result() is 1-indexed (tcnt+1), but this
message body still passes the 0-indexed value. A test skipping its first
case would produce:

  tst_test.c:1738: 1 TBROK: Test 0 haven't reported results!

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

Re: [LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[Petr Vorel]

Hi Andrea,

> Hi Petr,

> the idea is ok, but there are a few issues in the implementation.

> > +static unsigned int tcnt;

> `tcnt` is misleading because we are already using the same name inside
> `struct tst_test`. Better to use `tcase_idx` or something like this.

I deliberately chose tcnt because it's related to tst_test->tcnt. And test_all
is actually tcnt == 1. But sure we can name it tcase_idx.

> >  struct context {
> >  	int32_t lib_pid;
> > @@ -344,7 +345,7 @@ static void print_result(const char *file, const int lineno, int ttype,

> print_result() is used everywhere, by any tst_brk, tst_res, tst_vbrk_, tst_cvres
> etc. Which means you will have a `tcnt` printed everywhere. It looks better if
> we select only the starting message.

IMHO it's printed only on tst_brk() and tst_res() messages. tst_cvres() is
a special case of tst_vres_ called during test cleanup (TBROK changed to TWARN).

> >  		if (results_equal(&saved_results, results))
> > -			tst_brk(TBROK, "Test %i haven't reported results!", i);
> > +			tst_brk(TBROK, "Test %i haven't reported results!", tcnt);

> The new prefix printed by print_result() is 1-indexed (tcnt+1), but this
> message body still passes the 0-indexed value. A test skipping its first
> case would produce:

>   tst_test.c:1738: 1 TBROK: Test 0 haven't reported results!

Good point. FYI was also indexed by 0 before, but maybe we should really unify
it to start from 1. Or, really use 0 as it used to be, but I quite liked the
idea that 0 is for the setup function and tcnt + 1 is for the cleanup.
But looking that sometimes is the number really misleading:

$ ./test04
tst_test.c:2059: 1 TINFO: LTP version: 20260130-83-g4286eb4155
...
tst_test.c:1887: 1 TINFO: Overall timeout per run is 0h 00m 30s
test04.c:26: 1 TINFO: setup() executed by pid 147535
test04.c:16: 1 TPASS: PASSED message
test04.c:19: 2 TBROK: BROKEN message
tst_test.c:479: 2 TINFO: Child process reported TBROK killing the test
tst_test.c:1950: 1 TINFO: Killed the leftover descendant processes

=> it might be better if the number was printed really on tests, eg. [test 1],
maybe print also [setup] [cleanup] and [lib].

tst_test.c:2059: lib TINFO: LTP version: 20260130-83-g4286eb4155
...
tst_test.c:1887: [lib] TINFO: Overall timeout per run is 0h 00m 30s
test04.c:26: [setup] TINFO: setup() executed by pid 147535
test04.c:16: [test 1] TPASS: PASSED message
test04.c:19: [test 2] TBROK: BROKEN message
tst_test.c:479: [lib] TINFO: Child process reported TBROK killing the test
tst_test.c:1950: [lib]  TINFO: Killed the leftover descendant processes

Other thing to consider: do we want to print library and test forked child PID?
It might be useful when one sees somewhere in SUT logs coredump PID.
But quite a big part of each line would be occupied by this kind of prefix.
Maybe just printing once library and test PIDs would be enough.

Kind regards,
Petr

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

Re: [LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[Cyril Hrubis]

Hi!
> tst_test.c:1887: [lib] TINFO: Overall timeout per run is 0h 00m 30s
> test04.c:26: [setup] TINFO: setup() executed by pid 147535
> test04.c:16: [test 1] TPASS: PASSED message
> test04.c:19: [test 2] TBROK: BROKEN message
> tst_test.c:479: [lib] TINFO: Child process reported TBROK killing the test
> tst_test.c:1950: [lib]  TINFO: Killed the leftover descendant processes

This looks good to me. Maybe we can also color the setup/lib/test
differently if the output goes into the terminal.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[Cyril Hrubis]

Hi!
> > tst_test.c:1887: [lib] TINFO: Overall timeout per run is 0h 00m 30s
> > test04.c:26: [setup] TINFO: setup() executed by pid 147535
> > test04.c:16: [test 1] TPASS: PASSED message
> > test04.c:19: [test 2] TBROK: BROKEN message
> > tst_test.c:479: [lib] TINFO: Child process reported TBROK killing the test
> > tst_test.c:1950: [lib]  TINFO: Killed the leftover descendant processes
> 
> This looks good to me. Maybe we can also color the setup/lib/test
> differently if the output goes into the terminal.

And in order to make it work in the shell as well we would need a
function tst_set_context() that would set the current context so that we
can call it from shell. I suppose that this needs to be designed
carefuly in order to make ti work properly with threads/forked
processes.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[Petr Vorel]

> Hi!
> > > tst_test.c:1887: [lib] TINFO: Overall timeout per run is 0h 00m 30s
> > > test04.c:26: [setup] TINFO: setup() executed by pid 147535
> > > test04.c:16: [test 1] TPASS: PASSED message
> > > test04.c:19: [test 2] TBROK: BROKEN message
> > > tst_test.c:479: [lib] TINFO: Child process reported TBROK killing the test
> > > tst_test.c:1950: [lib]  TINFO: Killed the leftover descendant processes

> > This looks good to me. Maybe we can also color the setup/lib/test
> > differently if the output goes into the terminal.


> And in order to make it work in the shell as well we would need a
> function tst_set_context() that would set the current context so that we
> can call it from shell. I suppose that this needs to be designed
> carefuly in order to make ti work properly with threads/forked
> processes.

How about having enum with few states (lib, setup, test, cleanup), which would
be switched when context switches (and test_get_state() would just print the
string). And test would get the number (idea from this patch).

I'm still missing how to call C API code from shell test, because C API code
forks child and execute the script, how it could "talk back"?
The question I have asked in du01.sh rewrite:
https://lore.kernel.org/ltp/20260318154019.GE31214@pevik/

Kind regards,
Petr

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

Re: [LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[Cyril Hrubis]

Hi!
> > > This looks good to me. Maybe we can also color the setup/lib/test
> > > differently if the output goes into the terminal.
> 
> 
> > And in order to make it work in the shell as well we would need a
> > function tst_set_context() that would set the current context so that we
> > can call it from shell. I suppose that this needs to be designed
> > carefuly in order to make ti work properly with threads/forked
> > processes.
> 
> How about having enum with few states (lib, setup, test, cleanup), which would
> be switched when context switches (and test_get_state() would just print the
> string). And test would get the number (idea from this patch).
> 
> I'm still missing how to call C API code from shell test, because C API code
> forks child and execute the script, how it could "talk back"?

Same as the test results the enum would have to be stored in shared
memory and updated with atomic operations.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC PATCH 1/1] lib: Print tcnt in tst_{brk,res}()

[Petr Vorel]

> Hi!
> > > > This looks good to me. Maybe we can also color the setup/lib/test
> > > > differently if the output goes into the terminal.


> > > And in order to make it work in the shell as well we would need a
> > > function tst_set_context() that would set the current context so that we
> > > can call it from shell. I suppose that this needs to be designed
> > > carefuly in order to make ti work properly with threads/forked
> > > processes.

> > How about having enum with few states (lib, setup, test, cleanup), which would
> > be switched when context switches (and test_get_state() would just print the
> > string). And test would get the number (idea from this patch).

> > I'm still missing how to call C API code from shell test, because C API code
> > forks child and execute the script, how it could "talk back"?

> Same as the test results the enum would have to be stored in shared
> memory and updated with atomic operations.

Thanks for a hint, I should have figure this myself.

Anyway shell loader uses C API helper tst_res_.c anyway, shared memory is
directly available. But for the iterations (recently discussed) tst_run.sh will
need to have a simple C API helper to get required info.

Kind regards,
Petr

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