Re: [PATCH bpf-next v2 4/4] selftests/bpf: Add bpf_fib_lookup() VLAN flag tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Multi-line comments do not follow the mandatory BPF subsystem modern comment style.
--

commit ce4915a9efe53d10db6f263bbf7bb1a929ea9dbd
Author: Avinash Duduskar <avinash.duduskar@gmail.com>

selftests/bpf: Add bpf_fib_lookup() VLAN flag tests

This commit adds extensive test coverage for the new BPF_FIB_LOOKUP_VLAN and 
BPF_FIB_LOOKUP_VLAN_INPUT flags. It introduces 36 table cases and a new
cross-netns subtest to verify both the egress and input direction logic.

> diff --git a/tools/testing/selftests/bpf/prog_tests/fib_lookup.c b/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
> --- a/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fib_lookup.c

[ ... ]

> @@ -52,6 +88,16 @@ struct fib_lookup_test {
>  	__u32 tbid;
>  	__u8 dmac[6];
>  	__u32 mark;
> +	/* input tag with BPF_FIB_LOOKUP_VLAN_INPUT; expected output tag
> +	 * with BPF_FIB_LOOKUP_VLAN (checked when check_vlan is set)
> +	 */

[Severity: Low]
This isn't a bug, but does this follow the required BPF multi-line comment
style? The BPF subsystem prefers having the opening /* on its own line,
with the comment text starting on the next line.

[ ... ]

> @@ -204,6 +448,105 @@ static int setup_netns(void)
>  	SYS(fail, "ip rule add prio 2 fwmark %d lookup %s", MARK, MARK_TABLE);
>  	SYS(fail, "ip -6 rule add prio 2 fwmark %d lookup %s", MARK, MARK_TABLE);
>  
> +	/* Setup for vlan tests: a subinterface for egress resolution and
> +	 * tag-as-input, a QinQ stack, and an iif rule so the input tests
> +	 * observe which device the lookup used as ingress.
> +	 */

[Severity: Low]
This isn't a bug, but could we also adjust this block in setup_netns() to
place the opening /* on an empty line? There are several other multi-line
comments introduced in this patch that follow this older pattern.

[ ... ]

> +#define NS_VLAN_A	"fib_lookup_vlan_ns_a"
> +#define NS_VLAN_B	"fib_lookup_vlan_ns_b"
> +
> +/* A VLAN device can be moved to another netns while staying registered
> + * on its parent. Neither direction may then cross the boundary: the
> + * egress flag must not publish the foreign parent's ifindex, and the
> + * input flag must fail closed rather than use a foreign ingress.
> + */
> +void test_fib_lookup_vlan_netns(void)

[Severity: Low]
This isn't a bug, but could we apply the modern BPF multi-line comment 
style here as well before test_fib_lookup_vlan_netns() is declared?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616223426.3568080-1-avinash.duduskar@gmail.com?part=4