| Message ID | 20200904205657.27922-2-quentin@isovalent.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next,1/3] tools: bpftool: print optional built-in features along with version | expand |
On Fri, Sep 4, 2020 at 1:57 PM Quentin Monnet <quentin@isovalent.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? > } > > 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@isovalent.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? > } > return 0; > } > -- > 2.25.1 >
On 09/09/2020 00:20, Andrii Nakryiko wrote: > On Mon, Sep 7, 2020 at 7:50 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> On 04/09/2020 22:45, Andrii Nakryiko wrote: >>> On Fri, Sep 4, 2020 at 1:57 PM Quentin Monnet <quentin@isovalent.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@isovalent.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". > > Now you are making this into a list of potential features that could > be supported if only they were built with proper dependencies, don't > you think? (That will be the case for new versions of bpftool that will be able to dump their features, won't it? Anyway.) > > I thought the idea is to detect if a given bpftool that you have > supports, say, skeleton feature. Whether it's too old to support it or > it doesn't support because it wasn't built with necessary dependencies > is immaterial -- it doesn't support the feature, if there is no > "skeleton" in a list of features. I agree the reason does not matter, if the feature is not available then we cannot use it, period. The concern I have is false negatives. If a script does "bpftool version | grep libbfd" and gets no output, then it may skip dumping the JITed instructions, although bpftool might simply be too old to dump the feature. > > Continuing your logic -- parse JSON if you want to know this. In JSON > having {"skeleton": false, "libbfd": true"} feels natural. In > human-oriented plain text output seeing "features: libbpf=false, > skeleton=true" looks weird, instead of just "features: skeleton", IMO. Ok, so we agree we can have the booleans in JSON to have a way to avoid the "false negative" issue. In that case I'm fine with having a simpler list for plain output, this will make a small difference in the info provided between plain output and JSON but it seems acceptable. I'll send a new version with that change soon. Thanks, Quentin
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). -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"); } return 0; }
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 ] } 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@isovalent.com> --- tools/bpf/bpftool/Documentation/bpftool.rst | 8 +++++++- tools/bpf/bpftool/main.c | 22 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-)