Date:   Mon, 15 Mar 2021 22:16:37 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Quentin Monnet <quentin@...valent.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 08/11] bpftool: add `gen object` command to
 perform BPF static linking

On Mon, Mar 15, 2021 at 2:24 AM Quentin Monnet <quentin@...valent.com> wrote:
>
> 2021-03-13 11:35 UTC-0800 ~ Andrii Nakryiko <andrii@...nel.org>
> > Add `bpftool gen object <output-file> <input_file>...` command to statically
> > link multiple BPF ELF object files into a single output BPF ELF object file.
> >
> > Similarly to existing '*.o' convention, bpftool is establishing a '*.bpfo'
> > convention for statically-linked BPF object files. Both .o and .bpfo suffixes
> > will be stripped out during BPF skeleton generation to infer BPF object name.
> >
> > This patch also updates bash completions and man page. Man page gets a short
> > section on `gen object` command, but also updates the skeleton example to show
> > off workflow for BPF application with two .bpf.c files, compiled individually
> > with Clang, then resulting object files are linked together with `gen object`,
> > and then final object file is used to generate usable BPF skeleton. This
> > should help new users understand realistic workflow w.r.t. compiling
> > mutli-file BPF application.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> > ---
> >  .../bpf/bpftool/Documentation/bpftool-gen.rst | 65 +++++++++++++++----
> >  tools/bpf/bpftool/bash-completion/bpftool     |  2 +-
> >  tools/bpf/bpftool/gen.c                       | 49 +++++++++++++-
> >  3 files changed, 99 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> > index 84cf0639696f..4cdce187c393 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> > @@ -14,16 +14,37 @@ SYNOPSIS
> >
> >       *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] }
> >
> > -     *COMMAND* := { **skeleton** | **help** }
> > +     *COMMAND* := { **object** | **skeleton** | **help** }
> >
> >  GEN COMMANDS
> >  =============
> >
> > +|    **bpftool** **gen object** *OUTPUT_FILE* *INPUT_FILE* [*INPUT_FILE*...]
> >  |    **bpftool** **gen skeleton** *FILE*
> >  |    **bpftool** **gen help**
> >
> >  DESCRIPTION
> >  ===========
> > +     **bpftool gen object** *OUTPUT_FILE* *INPUT_FILE* [*INPUT_FILE*...]
> > +               Statically link (combine) together one or more *INPUT_FILE*'s
> > +               into a single resulting *OUTPUT_FILE*. All the files involed
>
> Typo: "involed"
>
> > +               are BPF ELF object files.
> > +
> > +               The rules of BPF static linking are mostly the same as for
> > +               user-space object files, but in addition to combining data
> > +               and instruction sections, .BTF and .BTF.ext (if present in
> > +               any of the input files) data are combined together. .BTF
> > +               data is deduplicated, so all the common types across
> > +               *INPUT_FILE*'s will only be represented once in the resulting
> > +               BTF information.
> > +
> > +               BPF static linking allows to partition BPF source code into
> > +               individually compiled files that are then linked into
> > +               a single resulting BPF object file, which can be used to
> > +               generated BPF skeleton (with **gen skeleton** command) or
> > +               passed directly into **libbpf** (using **bpf_object__open()**
> > +               family of APIs).
> > +
> >       **bpftool gen skeleton** *FILE*
> >                 Generate BPF skeleton C header file for a given *FILE*.
> >
>
> > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> > index fdffbc64c65c..7ca23c58f2c0 100644
> > --- a/tools/bpf/bpftool/bash-completion/bpftool
> > +++ b/tools/bpf/bpftool/bash-completion/bpftool
> > @@ -981,7 +981,7 @@ _bpftool()
> >              ;;
> >          gen)
> >              case $command in
> > -                skeleton)
> > +                object|skeleton)
> >                      _filedir
> >                      ;;
> >                  *)
>
> Suggesting the "object" keyword for completing "bpftool gen [tab]"
> is missing. It is just a few lines below:
>
> ------
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index fdffbc64c65c..223438e86932 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -981,12 +981,12 @@ _bpftool()
>              ;;
>          gen)
>              case $command in
> -                skeleton)
> +                object|skeleton)
>                      _filedir
>                      ;;
>                  *)
>                      [[ $prev == $object ]] && \
> -                        COMPREPLY=( $( compgen -W 'skeleton help' -- "$cur" ) )
> +                        COMPREPLY=( $( compgen -W 'object skeleton help' -- "$cur" ) )
>                      ;;
>              esac
>              ;;
> ------
>
> Looks good otherwise. Thanks for the documentation, it's great to
> have the example in the man page.
>
> Pending the two nits above are fixed:
> Reviewed-by: Quentin Monnet <quentin@...valent.com>

Thanks! I'll wait a bit for some more feedback on other patches and
will fix it in the next version (unless my other patches are perfect,
of course ;).

>
> Quentin

Powered by blists - more mailing lists