Re: [PATCH bpf-next v5 3/3] libbpf: add selftests for TC-BPF API

On Fri, Apr 30, 2021 at 11:34 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

On Sat, May 01, 2021 at 01:11:47AM IST, Andrii Nakryiko wrote:

quoted

On Wed, Apr 28, 2021 at 9:26 AM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

This adds some basic tests for the low level bpf_tc_* API.

Reviewed-by: Toke Høiland-Jørgensen <redacted>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../testing/selftests/bpf/prog_tests/tc_bpf.c | 467 ++++++++++++++++++
 .../testing/selftests/bpf/progs/test_tc_bpf.c |  12 +
 2 files changed, 479 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/tc_bpf.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf.c

[...]

quoted

quoted

+
+       /* attach */
+       ret = bpf_tc_attach(NULL, &attach_opts, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid hook = NULL"))
+               return -EINVAL;
+       ret = bpf_tc_attach(hook, &attach_opts, 42);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid flags"))
+               return -EINVAL;
+       attach_opts.prog_fd = 0;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid prog_fd unset"))
+               return -EINVAL;
+       attach_opts.prog_fd = fd;
+       attach_opts.prog_id = 42;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid prog_id set"))
+               return -EINVAL;
+       attach_opts.prog_id = 0;
+       attach_opts.handle = 0;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_OK(ret, "bpf_tc_attach valid handle unset"))
+               return -EINVAL;
+       attach_opts.prog_fd = attach_opts.prog_id = 0;
+       ASSERT_OK(bpf_tc_detach(hook, &attach_opts), "bpf_tc_detach");

this code is quite hard to follow, maybe sprinkle empty lines between
logical groups of statements (i.e., prepare inputs + call bpf_tc_xxx +
assert is one group that goes together)

I agree it looks bad. I can also just make a new opts for each combination, and
name it that way. Maybe that will look much better.

It probably would be just more code to read. Try to space it out with
empty lines into logical groups, that should be enough.

quoted

quoted

+       attach_opts.prog_fd = fd;
+       attach_opts.handle = 1;
+       attach_opts.priority = 0;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_OK(ret, "bpf_tc_attach valid priority unset"))
+               return -EINVAL;
+       attach_opts.prog_fd = attach_opts.prog_id = 0;
+       ASSERT_OK(bpf_tc_detach(hook, &attach_opts), "bpf_tc_detach");
+       attach_opts.prog_fd = fd;
+       attach_opts.priority = UINT16_MAX + 1;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid priority > UINT16_MAX"))
+               return -EINVAL;
+       attach_opts.priority = 0;
+       attach_opts.handle = attach_opts.priority = 0;
+       ret = bpf_tc_attach(hook, &attach_opts, 0);
+       if (!ASSERT_OK(ret, "bpf_tc_attach valid both handle and priority unset"))
+               return -EINVAL;
+       attach_opts.prog_fd = attach_opts.prog_id = 0;
+       ASSERT_OK(bpf_tc_detach(hook, &attach_opts), "bpf_tc_detach");
+       ret = bpf_tc_attach(hook, NULL, 0);
+       if (!ASSERT_EQ(ret, -EINVAL, "bpf_tc_attach invalid opts = NULL"))
+               return -EINVAL;
+
+       return 0;
+}
+

[...]