Sorry for the fragmented review. I should have been more thorough during
the first pass.
On Fri, Aug 28, 2020 at 11:48 AM CEST, Lorenz Bauer wrote:
> We compare socket cookies to ensure that insertion into a sockmap worked.
> Pull this out into a helper function for use in other tests.
>
> Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxxx>
> ---
> .../selftests/bpf/prog_tests/sockmap_basic.c | 51 ++++++++++++++-----
> 1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index 0b79d78b98db..b989f8760f1a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -47,6 +47,38 @@ static int connected_socket_v4(void)
> return -1;
> }
>
> +static void compare_cookies(struct bpf_map *src, struct bpf_map *dst)
> +{
> + __u32 i, max_entries = bpf_map__max_entries(src);
> + int err, duration, src_fd, dst_fd;
> +
> + src_fd = bpf_map__fd(src);
> + dst_fd = bpf_map__fd(src);
> +
> + for (i = 0; i < max_entries; i++) {
> + __u64 src_cookie, dst_cookie;
> +
> + err = bpf_map_lookup_elem(src_fd, &i, &src_cookie);
> + if (err && errno == ENOENT) {
> + err = bpf_map_lookup_elem(dst_fd, &i, &dst_cookie);
> + if (err && errno == ENOENT)
> + continue;
> +
> + CHECK(err, "map_lookup_elem(dst)", "element not deleted\n");
^^^
Here we want to fail if there was no error, i.e. lookup in dst
succeeded, or in the unlikely case there was some other error than
ENOENT.
> + continue;
> + }
> + if (CHECK(err, "lookup_elem(src, cookie)", "%s\n", strerror(errno)))
Nit: "lookup_elem(src)" as log tag would probably do. I don't see how
including the info that we're looking up a cookie helps.
> + continue;
> +
> + err = bpf_map_lookup_elem(dst_fd, &i, &dst_cookie);
> + if (CHECK(err, "lookup_elem(dst, cookie)", "%s\n", strerror(errno)))
> + continue;
> +
> + CHECK(dst_cookie != src_cookie, "cookie mismatch",
> + "%llu != %llu (pos %u)\n", dst_cookie, src_cookie, i);
Does it actually make sense to continue comparing the entries after the
first mismatch (or lookup error)? We could just fail-fast.
> + }
> +}
> +
[...]
