Re: [PATCH bpf-next v6 7/7] selftests/bpf: Add tests for XDP bonding

[Jussi Maki <joamaki@gmail.com>]

On Sat, Aug 7, 2021 at 12:50 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Aug 5, 2021 at 9:10 AM Jussi Maki <joamaki@gmail.com> wrote:
> >
> > Add a test suite to test XDP bonding implementation
> > over a pair of veth devices.
> >
> > Signed-off-by: Jussi Maki <joamaki@gmail.com>
> > ---
> >  .../selftests/bpf/prog_tests/xdp_bonding.c    | 520 ++++++++++++++++++
> >  1 file changed, 520 insertions(+)
> >
>
> I don't pretend to understand what's going on in this selftests, but
> it looks good from the generic selftest standpoint. One and half small
> issues below, please double-check (and probably fix the fd close
> issue).

Thanks for the reviews!

> > +       if (xdp_attach(skeletons,
> > +                      skeletons->xdp_redirect_multi_kern->progs.xdp_redirect_map_multi_prog,
> > +                      "bond2"))
> > +               goto out;
> > +
> > +       restore_root_netns();
>
> the "goto out" below might call restore_root_netns() again, is that ok?

Yep that's fine.

> > +       if (!test__start_subtest("xdp_bonding_redirect_multi"))
> > +               test_xdp_bonding_redirect_multi(&skeletons);
> > +
> > +out:
> > +       xdp_dummy__destroy(skeletons.xdp_dummy);
> > +       xdp_tx__destroy(skeletons.xdp_tx);
> > +       xdp_redirect_multi_kern__destroy(skeletons.xdp_redirect_multi_kern);
> > +
> > +       libbpf_set_print(old_print_fn);
> > +       if (root_netns_fd)
>
> technically, fd could be 0, so for fds we have if (fd >= 0)
> everywhere. Also, if open() above fails, root_netns_fd will be -1 and
> you'll still attempt to close it.

Good catch. Daniel, could you fix this when applying to be "if
(root_netns_fd >= 0)"?