Re: [PATCH v2 bpf-next 08/11] bpftool: add `gen object` command to perform BPF static linking

[Quentin Monnet <quentin@isovalent.com>]

2021-03-13 11:35 UTC-0800 ~ Andrii Nakryiko <andrii@kernel.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@kernel.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@isovalent.com>

Quentin