On Fri, Aug 28, 2020 at 9:59 AM <sdf@xxxxxxxxxx> wrote:
>
> On 08/25, Andrii Nakryiko wrote:
> > On Thu, Aug 20, 2020 at 2:44 AM YiFei Zhu <zhuyifei1999@xxxxxxxxx> wrote:
> > >
> > > From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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 :-/
Just because we made a mistake once, doesn't mean we need to keep
making it. ".metadata" is 9 characters already, which leaves 6
characters for object name prefix, that's not a lot of useful
information anyway. As I said, we should probably fix it for other
global data maps as well, but we will have to do it gradually. For
.metadata we can do a nice and clean ".metadata" immediately, no need
to jump through migration and deprecation hoops.
Also, how is "test_vml.bss" consistent with "test_v.metadata"?
