On Thu, 3 Sep 2020 at 06:35, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Sep 1, 2020 at 3:33 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> wrote:
> >
> > Add a test that exercises a basic sockmap / sockhash copy using bpf_iter.
> >
> > Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxxx>
> > ---
>
> just a bunch of nits, as I was passing by :-P
Appreciated, as always :)
>
> > .../selftests/bpf/prog_tests/sockmap_basic.c | 88 +++++++++++++++++++
> > tools/testing/selftests/bpf/progs/bpf_iter.h | 9 ++
> > .../selftests/bpf/progs/bpf_iter_sockmap.c | 58 ++++++++++++
> > .../selftests/bpf/progs/bpf_iter_sockmap.h | 3 +
> > 4 files changed, 158 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c
> > create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > index 9569bbac7f6e..f5b7b27f096f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > @@ -6,6 +6,9 @@
> > #include "test_skmsg_load_helpers.skel.h"
> > #include "test_sockmap_update.skel.h"
> > #include "test_sockmap_invalid_update.skel.h"
> > +#include "bpf_iter_sockmap.skel.h"
> > +
> > +#include "progs/bpf_iter_sockmap.h"
> >
> > #define TCP_REPAIR 19 /* TCP sock is under repair right now */
> >
> > @@ -196,6 +199,87 @@ static void test_sockmap_invalid_update(void)
> > test_sockmap_invalid_update__destroy(skel);
> > }
> >
> > +static void test_sockmap_copy(enum bpf_map_type map_type)
> > +{
> > + DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> > + int err, len, src_fd, iter_fd, duration;
> > + union bpf_iter_link_info linfo = {0};
>
> nit: misleading initialization, `= {}` is the same but doesn't imply
> that you can fill union/struct with non-zeroes like this
Ack.
>
> > + __s64 sock_fd[SOCKMAP_MAX_ENTRIES];
> > + __u32 i, num_sockets, max_elems;
> > + struct bpf_iter_sockmap *skel;
> > + struct bpf_map *src, *dst;
> > + struct bpf_link *link;
> > + char buf[64];
> > +
>
> [...]
>
> > +SEC("iter/sockmap")
> > +int copy_sockmap(struct bpf_iter__sockmap *ctx)
> > +{
> > + struct bpf_sock *sk = ctx->sk;
> > + __u32 tmp, *key = ctx->key;
> > + int ret;
> > +
> > + if (key == (void *)0)
>
> nit: seems like a verbose way to just write `if (!key)`?
Yeah, this is copypasta from the other iterator test. I'll change this.
>
> > + return 0;
> > +
> > + elems++;
> > +
> > + /* We need a temporary buffer on the stack, since the verifier doesn't
> > + * let us use the pointer from the context as an argument to the helper.
> > + */
> > + tmp = *key;
> > + bpf_printk("key: %u\n", tmp);
>
> is this intentional or a debugging leftover?
Oops!
>
> > +
> > + if (sk != (void *)0)
> > + return bpf_map_update_elem(&dst, &tmp, sk, 0) != 0;
> > +
> > + ret = bpf_map_delete_elem(&dst, &tmp);
> > + return ret && ret != -ENOENT;
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h
> > new file mode 100644
> > index 000000000000..f98ad727ac06
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h
> > @@ -0,0 +1,3 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define SOCKMAP_MAX_ENTRIES (64)
> > --
> > 2.25.1
> >
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
