Re: [PATCH bpf-next 4/5] bpftool: support dumping metadata

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

On Thu, Aug 20, 2020 at 2:44 AM YiFei Zhu <zhuyifei1999@gmail.com> wrote:
>
> From: YiFei Zhu <zhuyifei@google.com>
>
> Added a flag "--metadata" to `bpftool prog list` to dump the metadata
> contents. For some formatting some BTF code is put directly in the
> metadata dumping. Sanity checks on the map and the kind of the btf_type
> to make sure we are actually dumping what we are expecting.
>
> A helper jsonw_reset is added to json writer so we can reuse the same
> json writer without having extraneous commas.
>
> Sample output:
>
>   $ bpftool prog --metadata
>   6: cgroup_skb  name prog  tag bcf7977d3b93787c  gpl
>   [...]
>         btf_id 4
>         metadata:
>                 metadata_a = "foo"
>                 metadata_b = 1
>
>   $ bpftool prog --metadata --json --pretty
>   [{
>           "id": 6,
>   [...]
>           "btf_id": 4,
>           "metadata": {
>               "metadata_a": "foo",
>               "metadata_b": 1
>           }
>       }
>   ]
>
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> ---
>  tools/bpf/bpftool/json_writer.c |   6 ++
>  tools/bpf/bpftool/json_writer.h |   3 +
>  tools/bpf/bpftool/main.c        |  10 +++
>  tools/bpf/bpftool/main.h        |   1 +
>  tools/bpf/bpftool/prog.c        | 135 ++++++++++++++++++++++++++++++++
>  5 files changed, 155 insertions(+)
>

[...]

> +       for (i = 0; i < prog_info.nr_map_ids; i++) {
> +               map_fd = bpf_map_get_fd_by_id(map_ids[i]);
> +               if (map_fd < 0)
> +                       return;
> +
> +               err = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len);
> +               if (err)
> +                       goto out_close;
> +
> +               if (map_info.type != BPF_MAP_TYPE_ARRAY)
> +                       goto next_map;
> +               if (map_info.key_size != sizeof(int))
> +                       goto next_map;
> +               if (map_info.max_entries != 1)
> +                       goto next_map;
> +               if (!map_info.btf_value_type_id)
> +                       goto next_map;
> +               if (!strstr(map_info.name, ".metadata"))

This substring check sucks. Let's make libbpf call this map strictly
".metadata". Current convention of "some part of object name" + "." +
{rodata,data,bss} is extremely confusing. In practice it's something
incomprehensible and "unguessable" like "test_pr.rodata". I think it
makes sense to call them just ".data", ".rodata", ".bss", and
".metadata". But that might break existing apps that do lookups based
on map name (and might break skeleton as it is today, not sure). But
let's at least start with ".metadata", as it's a new map and we can
get it right from the start.

> +                       goto next_map;
> +
> +               goto found;
> +
> +next_map:
> +               close(map_fd);
> +       }
> +
> +       return;
> +
> +found:

[...]