On Tue, Oct 26, 2021 at 10:24 AM sunyucong@xxxxxxxxx
<sunyucong@xxxxxxxxx> wrote:
>
> On Mon, Oct 25, 2021 at 9:09 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 25, 2021 at 3:33 PM Yucong Sun <fallentree@xxxxxx> wrote:
> > >
> > > From: Yucong Sun <sunyucong@xxxxxxxxx>
> > >
> > > This patch restores behavior that prints one status line for each
> > > subtest executed. It works in both serial mode and parallel mode, and
> > > all verbosity settings.
> > >
> > > The logic around IO hijacking could use some more simplification in the
> > > future.
> > >
> >
> > This feels like a big hack, not a proper solution. What if we extend
> > MSG_TEST_DONE to signal also sub-test completion (along with subtest
> > logs)? Would that work better and result in cleaner logic?
>
> I think the current solution is actually cleaner. Yes we could add
I disagree. Subtest is a first-class citizen, even if it's harder to
parallelize due to more dynamic nature. Having protocol that reflects
the fact that test can consist of subtests is the right way to go, I
think.
> fields in task struct to record each subtest's name and status and
> generate the status line separately, but it will only work in
> situations where all tests pass.
> When there is an error, we do want to mix the status line with the
> actual stdout logs, which we won't be able to do afterwards.
I don't understand why success of failure is different. Worker
shouldn't log status line into the logs and should send subtest logs
separately from subtest name and status. And then dispatcher thread
will print log (depending on success/failure and/or verbosity
settings) and pre-defined status line. Status line is a fixed-format
thing that's derived from subtest name and its success/failure result.
It's not part of "log" per se.
>
> Besides, we will still need to implement separate logic in 3 places
> (serial mode, parallel mode in worker process, and serial part of
> parallel mode execution). Having two copies of stdout logs is actually
> not that bad.
Which is exactly why I was asking to reuse sequential loop, so that we
only have 2 loops. You felt it's not big deal. Maybe now is time to
rethink this?
Two copies of logs is certainly a design smell, so please give it some
thought and try to fix the protocol to accommodate subtests as a
concept.
>
> >
> > > Signed-off-by: Yucong Sun <sunyucong@xxxxxxxxx>
> > > ---
> > > tools/testing/selftests/bpf/test_progs.c | 56 +++++++++++++++++++-----
> > > tools/testing/selftests/bpf/test_progs.h | 4 ++
> > > 2 files changed, 50 insertions(+), 10 deletions(-)
> > >
[...]
> > > @@ -1357,6 +1387,12 @@ int main(int argc, char **argv)
> > >
> > > env.stdout = stdout;
> > > env.stderr = stderr;
> > > + env.subtest_status_fd = open_memstream(
> >
> > extremely misleading name, it's not an FD at all
>
> it is indeed a file descriptor, isn't it? What's a better name for it?
FD is an integer. This one is FILE *, so it may be "file", but
certainly not an fd.
>
> >
> > > + &env.subtest_status_buf, &env.subtest_status_cnt);
> > > + if (!env.subtest_status_fd) {
> > > + perror("Failed to setup env.subtest_status_fd");
> > > + exit(EXIT_ERR_SETUP_INFRA);
> > > + }
> > >
> > > env.has_testmod = true;
> > > if (!env.list_test_names && load_bpf_testmod()) {
> > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > > index 93c1ff705533..a564215a63b1 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.h
> > > +++ b/tools/testing/selftests/bpf/test_progs.h
> > > @@ -89,6 +89,10 @@ struct test_env {
> > > pid_t *worker_pids; /* array of worker pids */
> > > int *worker_socks; /* array of worker socks */
> > > int *worker_current_test; /* array of current running test for each worker */
> > > +
> > > + FILE* subtest_status_fd; /* fd for printing status line for subtests */
> > > + char *subtest_status_buf; /* buffer for subtests status */
> > > + size_t subtest_status_cnt;
> > > };
> > >
> > > #define MAX_LOG_TRUNK_SIZE 8192
> > > --
> > > 2.30.2
> > >
