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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: bpf <bpf@vger.kernel.org>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"KP Singh" <kpsingh@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	"Shaun Crampton" <shaun@tigera.io>,
	Networking <netdev@vger.kernel.org>
Subject: Re: [PATCH bpf-next v5 3/3] libbpf: add selftests for TC-BPF API
Date: Mon, 3 May 2021 15:55:39 -0700	[thread overview]
Message-ID: <CAEf4BzaZLEHF-Sg3H7Q5ERxPhw++ok0samjj9C4ET2ttXqztGQ@mail.gmail.com> (raw)
In-Reply-To: <20210501063436.fcts6od3ua2mxojl@apollo>

On Fri, Apr 30, 2021 at 11:34 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> 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@gmail.com> wrote:
> > >
> > > This adds some basic tests for the low level bpf_tc_* API.
> > >
> > > Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > > 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
> > >

[...]

> >
> > > +
> > > +       /* 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;
> > > +}
> > > +

[...]