Re: [PATCH bpf v2 2/2] selftests/bpf: Test FIONREAD on a sockmap socket without a verdict program

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The test allocates two socket pairs but only uses one.
- [Low] Multi-line comment violates BPF subsystem styling rules.
--

--- Patch [2]: [PATCH bpf v2 2/2] selftests/bpf: Test FIONREAD on a sockmap socket without a verdict program ---
commit b610a0eeb2389887cf749b47e6bba8ebcb416f08
Author: Mattia Meleleo <mattia.meleleo@coralogix.com>

selftests/bpf: Test FIONREAD on a sockmap socket without a verdict program

This commit adds a test to validate that FIONREAD on a TCP socket in a
sockmap without a verdict program correctly reports data pending in
sk_receive_queue.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index cb3229711f93a..f0f368201ddad 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -1373,6 +1373,43 @@ static void test_sockmap_multi_channels(int sotype)
>  	test_sockmap_pass_prog__destroy(skel);
>  }
>  
> +/* A socket in a sockmap without a verdict program keeps its ingress data
> + * in sk_receive_queue: FIONREAD must account for it.
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the BPF subsystem
coding style? The preferred style requires the opening /* to be on its own
line, rather than sharing a line with the comment text.

> +static void test_sockmap_no_verdict_fionread(void)
> +{
> +	int err, map, zero = 0, sent, avail;
> +	int c0 = -1, c1 = -1, p0 = -1, p1 = -1;
> +	struct test_sockmap_pass_prog *skel;
> +	char buf[256] = "0123456789";
> +
> +	skel = test_sockmap_pass_prog__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;
> +	map = bpf_map__fd(skel->maps.sock_map_rx);
> +
> +	err = create_socket_pairs(AF_INET, SOCK_STREAM, &c0, &c1, &p0, &p1);

[Severity: Low]
Is it necessary to allocate two socket pairs here? 

The test allocates c0 and p0 alongside c1 and p1, but c0 and p0 appear to be
entirely unused except for being closed at the end of the test. Could this
use create_pair() to allocate just a single socket pair instead?

> +	if (!ASSERT_OK(err, "create_socket_pairs()"))
> +		goto out;
> +
> +	err = bpf_map_update_elem(map, &zero, &c1, BPF_NOEXIST);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-fionread-no-verdict-v2-0-29dd293621c7@coralogix.com?part=2