Date:   Mon, 7 Sep 2020 15:50:36 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 1/3] tools: bpftool: print optional built-in
 features along with version

On 04/09/2020 22:45, Andrii Nakryiko wrote:
> On Fri, Sep 4, 2020 at 1:57 PM Quentin Monnet <quentin@...valent.com> wrote:
>>
>> Bpftool has a number of features that can be included or left aside
>> during compilation. This includes:
>>
>> - Support for libbfd, providing the disassembler for JIT-compiled
>>   programs.
>> - Support for BPF skeletons, used for profiling programs or iterating on
>>   the PIDs of processes associated with BPF objects.
>>
>> In order to make it easy for users to understand what features were
>> compiled for a given bpftool binary, print the status of the two
>> features above when showing the version number for bpftool ("bpftool -V"
>> or "bpftool version"). Document this in the main manual page. Example
>> invocation:
>>
>>     $ bpftool -p version
>>     {
>>         "version": "5.9.0-rc1",
>>         "features": [
>>             "libbfd": true,
>>             "skeletons": true
>>         ]
> 
> Is this a valid JSON? array of key/value pairs?

No it's not, silly me :'(. I'll fix that, thanks for spotting it.

>>     }
>>
>> Some other parameters are optional at compilation
>> ("DISASM_FOUR_ARGS_SIGNATURE", LIBCAP support) but they do not impact
>> significantly bpftool's behaviour from a user's point of view, so their
>> status is not reported.
>>
>> Available commands and supported program types depend on the version
>> number, and are therefore not reported either. Note that they are
>> already available, albeit without JSON, via bpftool's help messages.
>>
>> Signed-off-by: Quentin Monnet <quentin@...valent.com>
>> ---
>>  tools/bpf/bpftool/Documentation/bpftool.rst |  8 +++++++-
>>  tools/bpf/bpftool/main.c                    | 22 +++++++++++++++++++++
>>  2 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
>> index 420d4d5df8b6..a3629a3f1175 100644
>> --- a/tools/bpf/bpftool/Documentation/bpftool.rst
>> +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
>> @@ -50,7 +50,13 @@ OPTIONS
>>                   Print short help message (similar to **bpftool help**).
>>
>>         -V, --version
>> -                 Print version number (similar to **bpftool version**).
>> +                 Print version number (similar to **bpftool version**), and
>> +                 optional features that were included when bpftool was
>> +                 compiled. Optional features include linking against libbfd to
>> +                 provide the disassembler for JIT-ted programs (**bpftool prog
>> +                 dump jited**) and usage of BPF skeletons (some features like
>> +                 **bpftool prog profile** or showing pids associated to BPF
>> +                 objects may rely on it).
> 
> nit: I'd emit it as a list, easier to see list of features visually
> 
>>
>>         -j, --json
>>                   Generate JSON output. For commands that cannot produce JSON, this
>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>> index 4a191fcbeb82..2ae8c0d82030 100644
>> --- a/tools/bpf/bpftool/main.c
>> +++ b/tools/bpf/bpftool/main.c
>> @@ -70,13 +70,35 @@ static int do_help(int argc, char **argv)
>>
>>  static int do_version(int argc, char **argv)
>>  {
>> +#ifdef HAVE_LIBBFD_SUPPORT
>> +       const bool has_libbfd = true;
>> +#else
>> +       const bool has_libbfd = false;
>> +#endif
>> +#ifdef BPFTOOL_WITHOUT_SKELETONS
>> +       const bool has_skeletons = false;
>> +#else
>> +       const bool has_skeletons = true;
>> +#endif
>> +
>>         if (json_output) {
>>                 jsonw_start_object(json_wtr);
>> +
>>                 jsonw_name(json_wtr, "version");
>>                 jsonw_printf(json_wtr, "\"%s\"", BPFTOOL_VERSION);
>> +
>> +               jsonw_name(json_wtr, "features");
>> +               jsonw_start_array(json_wtr);
>> +               jsonw_bool_field(json_wtr, "libbfd", has_libbfd);
>> +               jsonw_bool_field(json_wtr, "skeletons", has_skeletons);
>> +               jsonw_end_array(json_wtr);
>> +
>>                 jsonw_end_object(json_wtr);
>>         } else {
>>                 printf("%s v%s\n", bin_name, BPFTOOL_VERSION);
>> +               printf("features: libbfd=%s, skeletons=%s\n",
>> +                      has_libbfd ? "true" : "false",
>> +                      has_skeletons ? "true" : "false");
> 
> now imagine parsing this with CLI text tools, you'll have to find
> "skeletons=(false|true)" and then parse "true" to know skeletons are
> supported. Why not just print out features that are supported?

You could just grep for "skeletons=true" (not too hard) (And generally
speaking I'd recommend against parsing bpftool's plain output, JSON is
more stable - Once you're parsing the JSON, checking the feature is
present or checking whether it's at "true" does not make a great
difference).

Anyway, the reason I have those booleans is that if you just list the
features and run "bpftool version | grep libbpfd" and get no result, you
cannot tell if the binary has been compiled without the disassembler or
if you are running an older version of bpftool that does not list
built-in features. You could then parse the version number and double
check, but you need to find in what version the change has been added.
Besides libbfd and skeletons, this could happen again for future
optional features if we add them to bpftool but forget to immediately
add the related check for "bpftool version".

So I would be inclined to keep the booleans. Or do you still believe a
list is preferable?

Quentin

Powered by blists - more mailing lists