On 28/08/2023 15:04, Hengqi Chen wrote: > Like maps and progs, add support to dump BTF > objects by name ([0]). Hi, thanks for looking into this! Can we also please support referencing by name for "bpftool btf list"? This will require collecting a list of the different programs with a matching name, like we do for "bpftool prog list name <foo>" (but dumps should still fail if there are more than one program matching, for consistency with "bpftool prog dump"). > > [0] Closes: https://github.com/libbpf/bpftool/issues/56 > > Signed-off-by: Hengqi Chen <hengqi.chen@xxxxxxxxx> > --- > tools/bpf/bpftool/btf.c | 92 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 91 insertions(+), 1 deletion(-) > > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c > index 91fcb75babe3..cb8d78ff4081 100644 > --- a/tools/bpf/bpftool/btf.c > +++ b/tools/bpf/bpftool/btf.c > @@ -547,6 +547,83 @@ static bool btf_is_kernel_module(__u32 btf_id) > return btf_info.kernel_btf && strncmp(btf_name, "vmlinux", sizeof(btf_name)) != 0; > } > > +static int btf_id_by_name(char *name, __u32 *btf_id) > +{ > + bool found = false; > + __u32 id = 0; > + int fd, err; > + > + while (true) { > + struct bpf_btf_info info = {}; > + __u32 len = sizeof(info); "info_len" instead of "len" would be more explicit, and this can probably be a const. > + char btf_name[64]; > + > + err = bpf_btf_get_next_id(id, &id); > + if (err) { > + if (errno == ENOENT) { > + if (found) > + err = 0; > + else > + p_err("no BTF object match name %s", name); > + break; > + } > + > + p_err("can't get next BTF object: %s%s", > + strerror(errno), > + errno == EINVAL ? " -- kernel too old?" : ""); > + return -1; > + } > + > + fd = bpf_btf_get_fd_by_id(id); > + if (fd < 0) { > + p_err("can't get BTF by id (%u): %s", > + id, strerror(errno)); > + return -1; > + } > + > + err = bpf_btf_get_info_by_fd(fd, &info, &len); > + if (err) { > + p_err("can't get BTF info (%u): %s", > + id, strerror(errno)); > + goto err_close_fd; > + } > + > + if (info.name_len) { > + memset(&info, 0, sizeof(info)); > + info.name_len = sizeof(btf_name); > + info.name = ptr_to_u64(btf_name); > + len = sizeof(info); sizeof(info) is the same as before, no need to reassign "len" (and we can use "len" in the memset()). > + > + err = bpf_btf_get_info_by_fd(fd, &info, &len); > + if (err) { > + p_err("can't get BTF info (%u): %s", > + id, strerror(errno)); > + goto err_close_fd; > + } > + } > + > + close(fd); > + > + if (strncmp(name, u64_to_ptr(info.name), BPF_OBJ_NAME_LEN)) There's no guarantee that "info.name" is set, here. Some BTF objects are anonymous and won't have a name. I'm getting a segfault from this strncmp() when trying the patch locally, because I've got anonymous BTF objects loaded and I end up passing a null pointer to the function. Can you please fix this? A follow-up question is how to handle anonymous objects. Given that we want to filter on names, we should probably allow empty name as well: "bpftool btf list name ''" should list anonymous objects, what do you think? > + continue; > + > + if (found) { > + p_err("multiple BTF object match name %s", name); > + return -1; > + } > + > + *btf_id = id; > + found = true; > + } > + > + return err; > + > +err_close_fd: > + close(fd); > + return err; > +} > + > + > static int do_dump(int argc, char **argv) > { > struct btf *btf = NULL, *base = NULL; > @@ -637,6 +714,19 @@ static int do_dump(int argc, char **argv) > *argv, strerror(errno)); > goto done; > } > + NEXT_ARG(); > + } else if (is_prefix(src, "name")) { > + char *name = *argv; > + > + if (strlen(name) > BPF_OBJ_NAME_LEN - 1) { > + p_err("can't parse name"); > + return -1; > + } > + > + err = btf_id_by_name(name, &btf_id); > + if (err) > + return -1; > + > NEXT_ARG(); > } else { > err = -1; > @@ -1062,7 +1152,7 @@ static int do_help(int argc, char **argv) > " %1$s %2$s dump BTF_SRC [format FORMAT]\n" > " %1$s %2$s help\n" > "\n" > - " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n" > + " BTF_SRC := { id BTF_ID | name NAME | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n" We will also need to update the command description in the relevant man page (.../Documentation/bpftool-btf.rst), and the bash completion (.../bash-completion/bpftool). Here's what I'd suggest for the bash completion: ------ diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool index 085bf18f3659..9aa0d2efe938 100644 --- a/tools/bpf/bpftool/bash-completion/bpftool +++ b/tools/bpf/bpftool/bash-completion/bpftool @@ -98,6 +98,12 @@ _bpftool_get_btf_ids() command sed -n 's/.*"id": \(.*\),$/\1/p' )" -- "$cur" ) ) } +_bpftool_get_btf_names() +{ + COMPREPLY+=( $( compgen -W "$( bpftool -jp btf 2>&1 | \ + command sed -n 's/.*"name": "\(.*\)",\?$/\1/p' )" -- "$cur" ) ) +} + _bpftool_get_link_ids() { COMPREPLY+=( $( compgen -W "$( bpftool -jp link 2>&1 | \ @@ -899,7 +905,7 @@ _bpftool() dump) case $prev in $command) - COMPREPLY+=( $( compgen -W "id map prog file" -- \ + COMPREPLY+=( $( compgen -W "id map name prog file" -- \ "$cur" ) ) return 0 ;; @@ -933,6 +939,9 @@ _bpftool() map) _bpftool_get_map_names ;; + $command) + _bpftool_get_btf_names + ;; esac return 0 ;; @@ -961,11 +970,14 @@ _bpftool() show|list) case $prev in $command) - COMPREPLY+=( $( compgen -W "id" -- "$cur" ) ) + COMPREPLY+=( $( compgen -W "id name" -- "$cur" ) ) ;; id) _bpftool_get_btf_ids ;; + name) + _bpftool_get_btf_names + ;; esac return 0 ;; ------ Please also make sure to Cc the other BPF maintainers for your next version. Thanks, Quentin
