On Fri, Apr 30, 2021 at 11:34 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote: > > On Sat, May 01, 2021 at 01:11:47AM IST, Andrii Nakryiko wrote: > > On Wed, Apr 28, 2021 at 9:26 AM Kumar Kartikeya Dwivedi > > <memxor@xxxxxxxxx> wrote: > > > > > > This adds some basic tests for the low level bpf_tc_* API. > > > > > > Reviewed-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> > > > --- > > > .../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 > > > [...] > > > > > + > > > + /* 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. > > > > + 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; > > > +} > > > + [...]
