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

On Thu, Sep 10, 2020 at 12:54 PM Andrii Nakryiko
[off-list ref] wrote:

On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev [off-list ref] wrote:

quoted

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.

quoted

+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.

quoted

+       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.