* Re: [PATCH bpf-next] bpftool: Support dumping BTF object by name
2023-08-28 14:04 [PATCH bpf-next] bpftool: Support dumping BTF object by name Hengqi Chen
@ 2023-08-30 10:26 ` Quentin Monnet
0 siblings, 0 replies; 2+ messages in thread
From: Quentin Monnet @ 2023-08-30 10:26 UTC (permalink / raw)
To: Hengqi Chen, bpf
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@gmail.com>
> ---
> 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
^ permalink raw reply related [flat|nested] 2+ messages in thread