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

[Stanislav Fomichev <sdf@google.com>]

On Thu, Sep 10, 2020 at 12:54 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev <sdf@google.com> wrote:
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index f7923414a052..ca264dc22434 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -29,6 +29,9 @@
> >  #include "main.h"
> >  #include "xlated_dumper.h"
> >
> > +#define BPF_METADATA_PREFIX "bpf_metadata_"
> > +#define BPF_METADATA_PREFIX_LEN strlen(BPF_METADATA_PREFIX)
>
> this is a runtime check, why not (sizeof(BPF_METADATA_PREFIX) - 1) instead?
Make sense, will fix.

> > +static int bpf_prog_find_metadata(int prog_fd, int *map_id)
> > ...
> > +free_map_ids:
> > +       saved_errno = errno;
> > +       free(map_ids);
> > +       errno = saved_errno;
>
> not clear why all this fussing with saving/restoring errno and then
> just returning 0 or -1? Just return -ENOMEM or -ENOENT as a result of
> this function?
Yeah, I just moved this function from it's original (libbpf) location as is.
I guess it makes sense to simplify the error handling now that
it's not in exported from libbpf.

> > +       if (bpf_map_lookup_elem(map_fd, &key, value))
> > +               goto out_free;
> > +
> > +       err = btf__get_from_id(map_info.btf_id, &btf);
> > +       if (err || !btf)
> > +               goto out_free;
>
> if you make bpf_prog_find_metadata() to do this value lookup and pass
> &info, it would probably make bpf_prog_find_metadata a bit more
> usable? You'll just need to ensure that callers free allocated memory.
> Then show_prog_metadata() would take care of processing BTF info.
Sounds reasonable. I can maybe keep existing
bpf_prog_find_metadata (rename to bpf_prog_find_metadata_map_id?)
and add another wrapper that does that map lookup.