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

[Petr Vorel <pvorel@suse.cz>]

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