Re: [PATCH v2 bpf-next 9/9] selftests/bpf: test kind encoding/decoding

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jun 16, 2023 at 10:18 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> verify btf__new_empty_opts() adds kind layouts for all kinds supported,
> and after adding kind-related types for an unknown kind, ensure that
> parsing uses this info when that kind is encountered rather than
> giving up.  Also verify that presence of a required kind will fail
> parsing.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  .../selftests/bpf/prog_tests/btf_kind.c       | 187 ++++++++++++++++++
>  1 file changed, 187 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/btf_kind.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_kind.c b/tools/testing/selftests/bpf/prog_tests/btf_kind.c
> new file mode 100644
> index 000000000000..ff37126b6bc0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_kind.c
> @@ -0,0 +1,187 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023, Oracle and/or its affiliates. */
> +
> +#include <test_progs.h>
> +#include <bpf/btf.h>
> +#include <bpf/libbpf.h>
> +
> +/* verify kind encoding exists for each kind */
> +void test_btf_kind_encoding(struct btf *btf)

static

> +{
> +       const struct btf_header *hdr;
> +       const void *raw_btf;
> +       __u32 raw_size;
> +
> +       raw_btf = btf__raw_data(btf, &raw_size);
> +       if (!ASSERT_OK_PTR(raw_btf, "btf__raw_data"))
> +               return;
> +
> +       hdr = raw_btf;
> +
> +       ASSERT_GT(hdr->kind_layout_off, hdr->str_off, "kind layout off");

check that it's multiple of 4 maybe?

> +       ASSERT_EQ(hdr->kind_layout_len, sizeof(struct btf_kind_layout) * NR_BTF_KINDS,
> +                 "kind_layout_len");
> +}
> +
> +static void write_raw_btf(const char *btf_path, void *raw_btf, size_t raw_size)
> +{
> +       int fd = open(btf_path, O_WRONLY | O_CREAT);
> +
> +       write(fd, raw_btf, raw_size);
> +       close(fd);
> +}

why bother with writing/reading to/from file, if you can just parse it
from memory with btf__new() ?

> +
> +/* fabricate an unrecognized kind at BTF_KIND_MAX + 1, and after adding
> + * the appropriate struct/typedefs to the BTF such that it recognizes
> + * this kind, ensure that parsing of BTF containing the unrecognized kind
> + * can succeed.
> + */
> +void test_btf_kind_decoding(struct btf *btf)
> +{
> +       __s32 int_id, unrec_id, id, id2;
> +       struct btf_type *t;
> +       char btf_path[64];
> +       const void *raw_btf;
> +       void *new_raw_btf;
> +       struct btf *new_btf;
> +       struct btf_header *hdr;
> +       struct btf_kind_layout *k;
> +       __u32 raw_size;
> +

[...]

> +
> +void test_btf_kind(void)
> +{
> +       LIBBPF_OPTS(btf_new_opts, opts);
> +
> +       opts.add_kind_layout = true;
> +
> +       struct btf *btf = btf__new_empty_opts(&opts);

are you trying to save 3 lines of code here but instead coupling
encoding/decoding subtests? Why? I had to go and check that there is
no expectation that test_btf_kind_encoding() has to be run first
before test_btf_kind_decoding(btf). Doesn't seem like there is, but
why doing this empty btf instantiation outside of each subtest? Keep
it simple, create empty btf inside the subtest as necessary.


> +
> +       if (!ASSERT_OK_PTR(btf, "btf_new"))
> +               return;
> +
> +       if (test__start_subtest("btf_kind_encoding"))
> +               test_btf_kind_encoding(btf);
> +       if (test__start_subtest("btf_kind_decoding"))
> +               test_btf_kind_decoding(btf);
> +       btf__free(btf);
> +}
> --
> 2.39.3
>