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

On 8/6/2026 12:29 PM, [email protected] wrote:
> 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.
> 

Good point. I will add an `if (!env.has_testmod)` check in v2 to gracefully
skip the test when `bpf_testmod` is unavailable.

Thanks for the feedback.

>> +		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.
> 

Thanks for the feedback. I will update the comment style in v2.

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