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

[bot+bpf-ci]

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

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.

> +	fd = bpf_link_create_log(bpf_program__fd(skel->progs.xdp_handler), IFINDEX_LO,
> +				 BPF_CGROUP_INET_INGRESS, NULL, &log_opts);

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39042048581