Re: [PATCH v2 bpf-next] bpf/selftests: add granular subtest output for prog_test

[Mykola Lysenko <mykolal@fb.com>]

> On Apr 27, 2022, at 3:38 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Apr 26, 2022 at 9:14 PM Mykola Lysenko <mykolal@fb.com> wrote:
>> 
>> Implement per subtest log collection for both parallel
>> and sequential test execution. This allows granular
>> per-subtest error output in the 'All error logs' section.
>> Add subtest log transfer into the protocol during the
>> parallel test execution.
>> 
>> Move all test log printing logic into dump_test_log
>> function. One exception is the output of test names when
>> verbose printing is enabled. Move test name/result
>> printing into separate functions to avoid repetition.
>> 
>> Print all successful subtest results in the log. Print
>> only failed test logs when test does not have subtests.
>> Or only failed subtests' logs when test has subtests.
>> 
>> Disable 'All error logs' output when verbose mode is
>> enabled. This functionality was already broken and is
>> causing confusion.
>> 
>> Signed-off-by: Mykola Lysenko <mykolal@fb.com>
>> ---
> 
> Works great! I've dropped the before test/subtest duplicated output of
> test/subtest name, as it seems unnecessary in practice. I dropped few
> lines of code that do that locally, as you suggested.
> 
> I also noticed a small memory leak, see comment below, please send a follow up.

Thanks a lot Andrii for the review!

I have sent a follow-up patch

> 
>> tools/testing/selftests/bpf/test_progs.c | 640 +++++++++++++++++------
>> tools/testing/selftests/bpf/test_progs.h | 35 +-
>> 2 files changed, 499 insertions(+), 176 deletions(-)
>> 
> 
> [...]
> 
>> +
>> +static int dispatch_thread_read_log(int sock_fd, char **log_buf, size_t *log_cnt)
>> +{
>> + FILE *log_fp = NULL;
>> +
>> + log_fp = open_memstream(log_buf, log_cnt);
>> + if (!log_fp)
>> + return 1;
>> +
>> + while (true) {
>> + struct msg msg;
>> +
>> + if (read_prog_test_msg(sock_fd, &msg, MSG_TEST_LOG))
> 
> leaking log_fp here?
> 
>> + return 1;
>> +
>> + fprintf(log_fp, "%s", msg.test_log.log_buf);
>> + if (msg.test_log.is_last)
>> + break;
>> + }
>> + fclose(log_fp);
>> + log_fp = NULL;
>> + return 0;
>> +}
>> +
> 
> [...]