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

From: sdf@google.com
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: YiFei Zhu <zhuyifei1999@gmail.com>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Mahesh Bandewar <maheshb@google.com>,
	YiFei Zhu <zhuyifei@google.com>
Subject: Re: [PATCH bpf-next 4/5] bpftool: support dumping metadata
Date: Fri, 28 Aug 2020 09:59:31 -0700	[thread overview]
Message-ID: <20200828165931.GA48607@google.com> (raw)
In-Reply-To: <CAEf4BzaGFP=Ob5MOcQgBjFOdY8aP1gvNV68wTAzA-V3kR5BKYg@mail.gmail.com>

On 08/25, Andrii Nakryiko wrote:
> 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.
Isn't it bad from the consistency point of view? Even if it's bad,
at least it's consistent :-/