Re: [PATCH bpf-next 2/2] selftests/bpf: Cover bpftool ring buffer event_pipe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The commit message claims to test 'pinned map selection', but it is never implemented or exercised in the test code.
- [Medium] The commit message claims to check 'pretty JSON output', but the test never invokes bpftool with the pretty print flag.
--

commit 7060e09603777e15c3fff0503b5038d8629bb63d
Author: Tianyi Chen <hi@tychen.cc>

selftests/bpf: Cover bpftool ring buffer event_pipe

This commit adds selftests for bpftool's ring buffer and perf event pipe
output. It aims to verify plain, JSON, and pretty JSON output formats, as
well as map selection methods and error handling.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/bpftool_ringbuf.c
> new file mode 100644
> index 0000000000000..2099bf1020930
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_ringbuf.c
[ ... ]
> @@ -0,0 +1,381 @@
[ ... ]
> +static bool consumer_start(struct consumer *child, int map_fd, bool json,
> +			   const char *option, bool pair, bool capture_errors)
> +{
> +	struct bpf_map_info info = {};
> +	__u32 len = sizeof(info);
> +	const char *path = bpftool_path();
> +	char *argv[12], id[16];
> +	int out[2], ready[2], n = 0, err, err_fd;
> +	struct pollfd pfd;
> +
> +	if (!ASSERT_OK_PTR(path, "bpftool path (set BPFTOOL to override)") ||
> +	    !ASSERT_OK(bpf_map_get_info_by_fd(map_fd, &info, &len), "map info"))
> +		return false;
> +	snprintf(id, sizeof(id), "%u", info.id);
> +	argv[n++] = (char *)path;
> +	if (json)
> +		argv[n++] = "-j";

[Severity: Medium]
The commit message states the test will check "complete plain, JSON and
pretty JSON output", but in consumer_start() the test only ever appends the
"-j" flag.

Is there a missing test case or flag assignment here to exercise the "-p"
(pretty JSON) bpftool output path as promised?

> +	argv[n++] = "map";
> +	argv[n++] = "event_pipe";
> +	argv[n++] = "id";
> +	argv[n++] = id;

[Severity: Medium]
The commit message claims to "Exercise ID and pinned map selection", but
consumer_start() hardcodes "id" for the map selection argument.

Is the logic to use "pinned" paths missing from this test implementation?

> +	if (option) {
> +		argv[n++] = (char *)option;
> +		argv[n++] = "0";
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906170747.1212378-1-hi@tychen.cc?part=2