Re: [RFC PATCH bpf-next 2/2] selftests/bpf: add tests for selective kmod BTF loading

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test `test_kmod_btfs` depends on the `bpf_testmod` kernel module being available, but does not check `env.has_testmod` before running.
- [Low] Multi-line comment does not follow BPF subsystem style guidelines.
--

--- Patch [2]: [RFC PATCH bpf-next 2/2] selftests/bpf: add tests for selective kmod BTF loading ---
commit 35262af12350c47550b117d06e881e0de422cb5d
Author: Fuyu Zhao <[email protected]>

selftests/bpf: add tests for selective kmod BTF loading

This commit adds test cases to verify the new .kmod_btfs section logic in
libbpf. It covers scenarios with valid, non-existent, and duplicated module
BTF targets to ensure robust loading and parsing.

> diff --git a/tools/testing/selftests/bpf/prog_tests/kmod_btfs.c b/tools/testing/selftests/bpf/prog_tests/kmod_btfs.c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/kmod_btfs.c
[ ... ]
> +static void kmod_btfs_mix(void)
> +{
> +	struct kmod_btfs_mix *kmod_btfs_mix_skel;
> +
> +	kmod_btfs_mix_skel = kmod_btfs_mix__open_and_load();
> +	if (!ASSERT_OK_PTR(kmod_btfs_mix_skel, "kmod_btfs_mix__open_and_load"))
> +		return;
> +
> +	kmod_btfs_mix__destroy(kmod_btfs_mix_skel);
> +}
> +
> +void test_kmod_btfs(void)
> +{
> +	if (test__start_subtest("kmod_btfs_pass"))

[Severity: Medium]
Since this test depends on bpf_testmod being available, should we check
env.has_testmod at the start of the test and skip it if not found?

Without it, running the selftests on a system where the test module is not
built or fails to load might cause erroneous test failures instead of being
gracefully skipped.

> +		kmod_btfs_pass();
> +
> +	if (test__start_subtest("kmod_btfs_nonexist"))
> +		kmod_btfs_nonexist();
> +
> +	if (test__start_subtest("kmod_btfs_mix"))
> +		kmod_btfs_mix();
> +}

> diff --git a/tools/testing/selftests/bpf/progs/kmod_btfs_nonexist.c b/tools/testing/selftests/bpf/progs/kmod_btfs_nonexist.c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kmod_btfs_nonexist.c
[ ... ]
> +SEC("fexit/bpf_testmod_loop_test")
> +int test_kmod_btfs_nonexist(void *ctx)
> +{
> +	return 0;
> +}
> +
> +/* This should fail to load, because .kmod_btfs does not contain
> + * the needed module 'bpf_testmod'.
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the BPF
subsystem style guidelines?

The BPF guidelines usually require the opening /* to be on its own line.

> +DEFINE_KMOD_BTFS(_needed_kmods) = { "module_nonexist" };
> +
> +char _license[] SEC("license") = "GPL";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2