On 2026/10/17 23:02, [email protected] 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
