Re: [PATCH bpf-next v9 8/8] selftests/bpf: Reuse stderr parsing for libarena ASAN tests

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Low] Unnecessary export of `free_msgs` to the global namespace.
- [Low] Use of a magic number for the BPF stream ID.
--

commit 49cffc0bea9678ea2577c634e093c46121bf87fd
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: Reuse stderr parsing for libarena ASAN tests

This commit adds code to directly test the output of libarena ASAN tests.
It reuses testing infrastructure originally for BPF streams to verify that
ASAN emits call stacks when memory errors occur.

> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index c4c34cae6102..ee637809a1d4 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
> @@ -93,7 +93,7 @@ void test_loader_fini(struct test_loader *tester)
>  	free(tester->log_buf);
>  }
>  
> -static void free_msgs(struct expected_msgs *msgs)
> +void free_msgs(struct expected_msgs *msgs)

Are there any callers outside of test_loader.c that need this function?

It appears the newly added verify_test_stderr() encapsulates the cleanup
internally by calling free_test_spec(), leaving free_msgs() without any
external callers. Could this function remain static to avoid unnecessary
namespace pollution?

[ ... ]

> @@ -789,6 +789,43 @@ static void emit_stderr(const char *stderr, bool force)
>  	fprintf(stdout, "STDERR:\n=============\n%s=============\n", stderr);
>  }
>  
> +static void verify_stderr(int prog_fd, struct expected_msgs *msgs)
> +{
> +	LIBBPF_OPTS(bpf_prog_stream_read_opts, ropts);
> +	char *buf;
> +	int ret;
> +
> +	if (!msgs->cnt)
> +		return;
> +
> +	buf = malloc(TEST_LOADER_LOG_BUF_SZ);
> +	if (!ASSERT_OK_PTR(buf, "malloc"))
> +		return;
> +
> +	ret = bpf_prog_stream_read(prog_fd, 2, buf, TEST_LOADER_LOG_BUF_SZ - 1,
> +				    &ropts);

Would it be better to use the BPF_STREAM_STDERR macro here instead of
hardcoding the value 2?

Using the macro defined in include/uapi/linux/bpf.h might improve
readability and stay consistent with other stream testing code.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 1a44467f4310..37955a8ad385 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -563,5 +563,7 @@ struct expected_msgs {
>  
>  void validate_msgs(const char *log_buf, struct expected_msgs *msgs,
>  		   void (*emit_fn)(const char *buf, bool force));
> +void free_msgs(struct expected_msgs *msgs);

If free_msgs() can remain static, should this declaration be removed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260426190338.4615-1-emil@etsalapatis.com?part=8