Re: [PATCH bpf-next 2/3] selftests: bpf: Add helper to compare socket cookies

[Jakub Sitnicki <jakub@cloudflare.com>]

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@cloudflare.com>
> ---
>  .../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.

> +	}
> +}
> +

[...]