Re: [PATCH bpf-next 6/6] selftests/bpf: Add tests to verify link_create logs

[Leon Hwang]

On 2026/10/17 23:02, bot+bpf-ci@xxxxxxxxxx wrote:
>> selftests/bpf: Add tests to verify link_create logs
> 
> TL;DR: Nits only, in the new link_create log selftests in xdp_link.c: the four
> subtests repeat the same setup and assertions and could share a helper, and
> one has a redundant true_size = 0.
> 
>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_link.c b/tools/testing/selftests/bpf/prog_tests/xdp_link.c
>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_link.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_link.c
> 
> [ ... ]
> 
>> @@ -150,3 +150,135 @@ void serial_test_xdp_link(void)
>>  	test_xdp_link__destroy(skel1);
>>  	test_xdp_link__destroy(skel2);
>>  }
>> +
>> +static void test_link_create_invalid_prog_fd(void)
>> +{
>> +	const char *msg = "Invalid prog_fd.\n";
>> +	char log_buf[128] = {};
>> +	int fd;
>> +	LIBBPF_OPTS(bpf_log_opts, log_opts,
>> +		    .buf = log_buf,
>> +		    .size = sizeof(log_buf),
>> +		    .level = 1,
>> +	);
>> +
>> +	fd = bpf_link_create_log(-1, IFINDEX_LO, BPF_XDP, NULL, &log_opts);
>> +	if (!ASSERT_LT(fd, 0, "bpf_link_create_log")) {
>> +		close(fd);
>> +		return;
>> +	}
>> +
>> +	ASSERT_EQ(fd, -EBADF, "fd");
>> +	ASSERT_STREQ(log_buf, msg, "log_buf");
>> +	ASSERT_EQ(log_opts.true_size, strlen(msg) + 1, "log_size");
>> +}
> 
> This isn't a bug, but the four new test_link_create_*() functions repeat the
> same log_buf and LIBBPF_OPTS(bpf_log_opts, ...) setup, and the same trailing
> ASSERT_EQ(fd, ...) / ASSERT_STREQ(log_buf, msg) /
> ASSERT_EQ(log_opts.true_size, strlen(msg) + 1) sequence.  They differ only in
> prog fd, target, attach type, expected errno and message.

Will factor out helpers to simplify the tests code.

> 
> This looks close to test_map_create() in map_init.c.  Could the four
> link_create log tests share a small helper that takes the prog fd, target,
> attach type, opts, expected errno and message?
> 
>> +
>> +static void test_link_create_invalid_attach_type(void)
>> +{
>> +	const char *msg = "Invalid attach_type.\n";
>> +	struct test_xdp_link *skel;
>> +	char log_buf[128] = {};
>> +	int fd;
>> +	LIBBPF_OPTS(bpf_log_opts, log_opts,
>> +		    .buf = log_buf,
>> +		    .size = sizeof(log_buf),
>> +		    .level = 1,
>> +	);
>> +
>> +	skel = test_xdp_link__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "test_xdp_link__open_and_load"))
>> +		return;
>> +
>> +	log_opts.true_size = 0;
> 
> This isn't a bug, but is the log_opts.true_size = 0; line needed here?
> LIBBPF_OPTS() already zero-initializes log_opts, true_size is only written by
> libbpf after the syscall, and the other three tests omit this line.

Good catch.

This line is unnecessary. Will drop it in the next revision.

Thanks,
Leon