Hi Quentin,
On Mon, Dec 09, 2024 at 12:26:50PM GMT, Quentin Monnet wrote:
> On 07/12/2024 23:24, Daniel Xu wrote:
> > Some projects, for example xdp-tools [0], prefer to check in a minimized
> > vmlinux.h rather than the complete file which can get rather large.
> >
> > However, when you try to add a minimized version of a complex struct (eg
> > struct xfrm_state), things can get quite complex if you're trying to
> > manually untangle and deduplicate the dependencies.
> >
> > This commit teaches bpftool to do a minimized dump of a single type by
> > providing an optional root_id argument.
> >
> > Example usage:
> >
> > $ ./bpftool btf dump file ~/dev/linux/vmlinux | rg "STRUCT 'xfrm_state'"
> > [12643] STRUCT 'xfrm_state' size=912 vlen=58
> >
> > $ ./bpftool btf dump file ~/dev/linux/vmlinux root_id 12643 format c
> > #ifndef __VMLINUX_H__
> > #define __VMLINUX_H__
> >
> > [..]
> >
> > struct xfrm_type_offload;
> >
> > struct xfrm_sec_ctx;
> >
> > struct xfrm_state {
> > possible_net_t xs_net;
> > union {
> > struct hlist_node gclist;
> > struct hlist_node bydst;
> > };
> > union {
> > struct hlist_node dev_gclist;
> > struct hlist_node bysrc;
> > };
> > struct hlist_node byspi;
> > [..]
> >
> > [0]: https://github.com/xdp-project/xdp-tools/blob/master/headers/bpf/vmlinux.h
> >
> > Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
> > ---
> > Changes in v2:
> > * Add early error check for invalid BTF ID
> >
> > .../bpf/bpftool/Documentation/bpftool-btf.rst | 5 +++--
> > tools/bpf/bpftool/btf.c | 19 +++++++++++++++++++
> > 2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > index 3f6bca03ad2e..5abd0e99022f 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > @@ -27,7 +27,7 @@ BTF COMMANDS
> > | **bpftool** **btf dump** *BTF_SRC* [**format** *FORMAT*]
> > | **bpftool** **btf help**
> > |
> > -| *BTF_SRC* := { **id** *BTF_ID* | **prog** *PROG* | **map** *MAP* [{**key** | **value** | **kv** | **all**}] | **file** *FILE* }
> > +| *BTF_SRC* := { **id** *BTF_ID* | **prog** *PROG* | **map** *MAP* [{**key** | **value** | **kv** | **all**}] | **file** *FILE* [**root_id** *ROOT_ID*] }
>
>
> Thanks for this!
No problem!
>
> root_id is not part of the BTF_SRC, I think it should be an option on
> the command line itself (3 lines above), after "format". And the change
> should also be repeated below in the description (the "format" option is
> missing, by the way, let's fix it too).
Sure, will do. I was originally thinking it would conflict with `map`
subcommand, as it also sets the root_type_ids, but we can just error out
if root_type_cnt > 0 and root_id is pased.
Will send v3 with below suggestions as well.
>
> Can you please also update the interactive help message at the end of
> btf.c?
>
> Can you please also update the bash completion file? I think it should
> look like this:
>
>
> ------
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 0c541498c301..097d406ee21f 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -930,6 +930,9 @@ _bpftool()
> format)
> COMPREPLY=( $( compgen -W "c raw" -- "$cur" ) )
> ;;
> + root_id)
> + return 0;
> + ;;
> c)
> COMPREPLY=( $( compgen -W "unsorted" -- "$cur" ) )
> ;;
> @@ -937,13 +940,13 @@ _bpftool()
> # emit extra options
> case ${words[3]} in
> id|file)
> - _bpftool_once_attr 'format'
> + _bpftool_once_attr 'format root_id'
> ;;
> map|prog)
> if [[ ${words[3]} == "map" ]] && [[ $cword == 6 ]]; then
> COMPREPLY+=( $( compgen -W "key value kv all" -- "$cur" ) )
> fi
> - _bpftool_once_attr 'format'
> + _bpftool_once_attr 'format root_id'
> ;;
> *)
> ;;
> ------
>
>
> > | *FORMAT* := { **raw** | **c** [**unsorted**] }
> > | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > | *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* }
> > @@ -60,7 +60,8 @@ bpftool btf dump *BTF_SRC*
> >
> > When specifying *FILE*, an ELF file is expected, containing .BTF section
> > with well-defined BTF binary format data, typically produced by clang or
> > - pahole.
> > + pahole. You can choose to dump a single type and all its dependent types
> > + by providing an optional *ROOT_ID*.
> >
> > **format** option can be used to override default (raw) output format. Raw
> > (**raw**) or C-syntax (**c**) output formats are supported. With C-style
> > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> > index d005e4fd6128..a75e17efaf5e 100644
> > --- a/tools/bpf/bpftool/btf.c
> > +++ b/tools/bpf/bpftool/btf.c
> > @@ -953,6 +953,8 @@ static int do_dump(int argc, char **argv)
> > NEXT_ARG();
> > } else if (is_prefix(src, "file")) {
> > const char sysfs_prefix[] = "/sys/kernel/btf/";
> > + __u32 root_id;
> > + char *end;
>
>
> I think we could move these declarations to a lower scope, under your
> "if (argc && is_prefix(...))".
>
>
> >
> > if (!base_btf &&
> > strncmp(*argv, sysfs_prefix, sizeof(sysfs_prefix) - 1) == 0 &&
> > @@ -967,6 +969,23 @@ static int do_dump(int argc, char **argv)
> > goto done;
> > }
> > NEXT_ARG();
> > +
> > + if (argc && is_prefix(*argv, "root_id")) {
> > + NEXT_ARG();
> > + root_id = strtoul(*argv, &end, 0);
> > + if (*end) {
> > + err = -1;
> > + p_err("can't parse %s as root ID", *argv);
> > + goto done;
> > + }
> > + if (root_id >= btf__type_cnt(btf)) {
> > + err = -EINVAL;
> > + p_err("invalid root ID: %u", root_id);
> > + goto done;
> > + }
> > + root_type_ids[root_type_cnt++] = root_id;
> > + NEXT_ARG();
> > + }
> > } else {
> > err = -1;
> > p_err("unrecognized BTF source specifier: '%s'", src);
>
>
> Same comment as above, it seems to be that the root_id controls the
> output for the command rather than the source, and I'd rather move this
> to the "while (argc)" loop where we process the "format" option rather
> than when parsing the source.
>
>
> pw-bot: cr
