Re: [PATCH bpf-next v2 6/7] selftests/bpf: Skip expected_msgs subtests when stdout is not saved

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

> selftests/bpf: Skip expected_msgs subtests when stdout is not saved

TL;DR: Nit only: the new test__skip() in the expected_msgs selftest is silent,
so -v output doesn't say why subtests were skipped; the review asks whether to
print a short reason. The skip logic itself looks correct.

> diff --git a/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c b/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c
> index 53347f379924..b4cd7c7ff439 100644
> --- a/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c
> +++ b/tools/testing/selftests/bpf/prog_tests/prog_tests_framework.c

[ ... ]

> @@ -169,6 +169,12 @@ void test_prog_tests_framework_expected_msgs(void)
>
>  	for (i = 0; i < ARRAY_SIZE(cases); i++) {
>  		if (test__start_subtest(cases[i].name)) {
> +			if (!env.subtest_state->stdout_saved) {
> +				test__skip();
> +				test__end_subtest();
> +				continue;
> +			}
> +

This isn't a bug, but a subsystem guideline flags a test__skip() that is
issued silently, with no message.

test__skip() here is called without any explanation, so a -v run only shows
the generic "#N/M <subtest>:SKIP" line from test__end_subtest().  Someone
reading that output has to look at the code or the commit message to learn
the subtests were skipped because stdout is not captured in non-parallel
verbose mode.

The skip itself looks correct, since stdio_hijack_init() returns early when
verbose() && env.worker_id == -1 and leaves subtest_state->stdout_saved and
log_buf unset.  test__skip() takes no reason argument, and many other
test__skip() callers in prog_tests are also bare.

Would it be worth printing a short reason before test__skip(), for example:

    fprintf(stderr, "SKIP: stdout is not saved in non-parallel verbose mode\n");

>  			error_cnt = env.subtest_state->error_cnt;
>  			msgs.patterns = cases[i].pats;
>  			msgs.cnt = 0;


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/38559567568