[PATCH bpf-next 0/3] tools: bpftool: print built-in features, automate some of the documentation

bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH bpf-next 0/3] tools: bpftool: print built-in features, automate some of the documentation
@ 2020-09-04 20:56 Quentin Monnet
  2020-09-04 20:56 ` [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version Quentin Monnet
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Quentin Monnet @ 2020-09-04 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann; +Cc: bpf, netdev, Quentin Monnet

There are two changes for bpftool in this series.

The first one is a modification to the "version" command, to have it print
the status (compiled or not) of some of the optional features for bpftool.
This is to help determine if a bpftool binary is able to, for example,
disassemble JIT-compiled programs.

The last two patches try to automate the generation of some repetitive
sections in the man pages for bpftool, namely the description of the
options shared by all commands, and the "see also" section. The objective
is to make it easier to maintain the pages and to reduce the risk of
omissions when adding the documentation for new commands.

Quentin Monnet (3):
  tools: bpftool: print optional built-in features along with version
  tools: bpftool: include common options from separate file
  tools: bpftool: automate generation for "SEE ALSO" sections in man
    pages

 tools/bpf/bpftool/Documentation/Makefile      | 14 ++++++--
 .../bpf/bpftool/Documentation/bpftool-btf.rst | 34 +-----------------
 .../bpftool/Documentation/bpftool-cgroup.rst  | 33 +----------------
 .../bpftool/Documentation/bpftool-feature.rst | 33 +----------------
 .../bpf/bpftool/Documentation/bpftool-gen.rst | 33 +----------------
 .../bpftool/Documentation/bpftool-iter.rst    | 27 +-------------
 .../bpftool/Documentation/bpftool-link.rst    | 34 +-----------------
 .../bpf/bpftool/Documentation/bpftool-map.rst | 33 +----------------
 .../bpf/bpftool/Documentation/bpftool-net.rst | 34 +-----------------
 .../bpftool/Documentation/bpftool-perf.rst    | 34 +-----------------
 .../bpftool/Documentation/bpftool-prog.rst    | 34 +-----------------
 .../Documentation/bpftool-struct_ops.rst      | 35 +------------------
 tools/bpf/bpftool/Documentation/bpftool.rst   | 34 +-----------------
 .../bpftool/Documentation/common_options.rst  | 22 ++++++++++++
 tools/bpf/bpftool/main.c                      | 22 ++++++++++++
 15 files changed, 68 insertions(+), 388 deletions(-)
 create mode 100644 tools/bpf/bpftool/Documentation/common_options.rst

-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version
  2020-09-04 20:56 [PATCH bpf-next 0/3] tools: bpftool: print built-in features, automate some of the documentation Quentin Monnet
@ 2020-09-04 20:56 ` Quentin Monnet
  2020-09-04 21:45   ` Andrii Nakryiko
  2020-09-04 20:56 ` [PATCH bpf-next 2/3] tools: bpftool: include common options from separate file Quentin Monnet
  2020-09-04 20:56 ` [PATCH bpf-next 3/3] tools: bpftool: automate generation for "SEE ALSO" sections in man pages Quentin Monnet
  2 siblings, 1 reply; 11+ messages in thread
From: Quentin Monnet @ 2020-09-04 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann; +Cc: bpf, netdev, Quentin Monnet

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(-)

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;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version
  2020-09-04 20:56 ` [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version Quentin Monnet
@ 2020-09-04 21:45   ` Andrii Nakryiko
  2020-09-07 14:50     ` Quentin Monnet
  0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-09-04 21:45 UTC (permalink / raw)
  To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version
  2020-09-04 21:45   ` Andrii Nakryiko
@ 2020-09-07 14:50     ` Quentin Monnet
  2020-09-08 23:20       ` Andrii Nakryiko
  0 siblings, 1 reply; 11+ messages in thread
From: Quentin Monnet @ 2020-09-07 14:50 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking

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".

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

Quentin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version
  2020-09-07 14:50     ` Quentin Monnet
@ 2020-09-08 23:20       ` Andrii Nakryiko
  2020-09-09  8:35         ` Quentin Monnet
  0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-09-08 23:20 UTC (permalink / raw)
  To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking

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?

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.

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.

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

I still believe plain text should be minimal and simple, but won't
argue further :)

> Quentin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version
  2020-09-08 23:20       ` Andrii Nakryiko
@ 2020-09-09  8:35         ` Quentin Monnet
  0 siblings, 0 replies; 11+ messages in thread
From: Quentin Monnet @ 2020-09-09  8:35 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH bpf-next 2/3] tools: bpftool: include common options from separate file
  2020-09-04 20:56 [PATCH bpf-next 0/3] tools: bpftool: print built-in features, automate some of the documentation Quentin Monnet
  2020-09-04 20:56 ` [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version Quentin Monnet
@ 2020-09-04 20:56 ` Quentin Monnet
  2020-09-04 21:46   ` Andrii Nakryiko
  2020-09-04 20:56 ` [PATCH bpf-next 3/3] tools: bpftool: automate generation for "SEE ALSO" sections in man pages Quentin Monnet
  2 siblings, 1 reply; 11+ messages in thread
From: Quentin Monnet @ 2020-09-04 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann; +Cc: bpf, netdev, Quentin Monnet

Nearly all man pages for bpftool have the same common set of option
flags (--help, --version, --json, --pretty, --debug). The description is
duplicated across all the pages, which is more difficult to maintain if
the description of an option changes. It may also be confusing to sort
out what options are not "common" and should not be copied when creating
new manual pages.

Let's move the description for those common options to a separate file,
which is included with a RST directive when generating the man pages.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/Documentation/Makefile      |  2 +-
 .../bpf/bpftool/Documentation/bpftool-btf.rst | 17 +------------
 .../bpftool/Documentation/bpftool-cgroup.rst  | 17 +------------
 .../bpftool/Documentation/bpftool-feature.rst | 17 +------------
 .../bpf/bpftool/Documentation/bpftool-gen.rst | 17 +------------
 .../bpftool/Documentation/bpftool-iter.rst    | 11 +--------
 .../bpftool/Documentation/bpftool-link.rst    | 17 +------------
 .../bpf/bpftool/Documentation/bpftool-map.rst | 17 +------------
 .../bpf/bpftool/Documentation/bpftool-net.rst | 17 +------------
 .../bpftool/Documentation/bpftool-perf.rst    | 17 +------------
 .../bpftool/Documentation/bpftool-prog.rst    | 18 +-------------
 .../Documentation/bpftool-struct_ops.rst      | 18 +-------------
 tools/bpf/bpftool/Documentation/bpftool.rst   | 24 +------------------
 .../bpftool/Documentation/common_options.rst  | 22 +++++++++++++++++
 14 files changed, 35 insertions(+), 196 deletions(-)
 create mode 100644 tools/bpf/bpftool/Documentation/common_options.rst

diff --git a/tools/bpf/bpftool/Documentation/Makefile b/tools/bpf/bpftool/Documentation/Makefile
index 815ac9804aee..becbb8c52257 100644
--- a/tools/bpf/bpftool/Documentation/Makefile
+++ b/tools/bpf/bpftool/Documentation/Makefile
@@ -19,7 +19,7 @@ man8dir = $(mandir)/man8
 # Load targets for building eBPF helpers man page.
 include ../../Makefile.helpers
 
-MAN8_RST = $(filter-out $(HELPERS_RST),$(wildcard *.rst))
+MAN8_RST = $(wildcard bpftool*.rst)
 
 _DOC_MAN8 = $(patsubst %.rst,%.8,$(MAN8_RST))
 DOC_MAN8 = $(addprefix $(OUTPUT),$(_DOC_MAN8))
diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
index 896f4c6c2870..0020bb55cf7e 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
@@ -71,22 +71,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
-
-	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
+	.. include:: common_options.rst
 
 EXAMPLES
 ========
diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index a226aee3574f..3dba89db000e 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -116,26 +116,11 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
+	.. include:: common_options.rst
 
 	-f, --bpffs
 		  Show file names of pinned programs.
 
-	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
-
 EXAMPLES
 ========
 |
diff --git a/tools/bpf/bpftool/Documentation/bpftool-feature.rst b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
index 8609f06e71de..f1aae5690e3c 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-feature.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
@@ -71,22 +71,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
-
-	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
+	.. include:: common_options.rst
 
 SEE ALSO
 ========
diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
index df85dbd962c0..e3b7ff3c09d7 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-gen.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
@@ -126,22 +126,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON,
-		  this option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
-
-	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
+	.. include:: common_options.rst
 
 EXAMPLES
 ========
diff --git a/tools/bpf/bpftool/Documentation/bpftool-iter.rst b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
index 070ffacb42b5..b688cf11805c 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-iter.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
@@ -51,16 +51,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-d, --debug
-		  Print all logs available, even debug-level information. This
-		  includes logs from libbpf as well as from the verifier, when
-		  attempting to load programs.
+	.. include:: common_options.rst
 
 EXAMPLES
 ========
diff --git a/tools/bpf/bpftool/Documentation/bpftool-link.rst b/tools/bpf/bpftool/Documentation/bpftool-link.rst
index 4a52e7a93339..001689efc7fa 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-link.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-link.rst
@@ -62,18 +62,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
+	.. include:: common_options.rst
 
 	-f, --bpffs
 		  When showing BPF links, show file names of pinned
@@ -83,10 +72,6 @@ OPTIONS
 		  Do not automatically attempt to mount any virtual file system
 		  (such as tracefs or BPF virtual file system) when necessary.
 
-	-d, --debug
-		  Print all logs available, even debug-level information. This
-		  includes logs from libbpf.
-
 EXAMPLES
 ========
 **# bpftool link show**
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 083db6c2fc67..e06a65cd467e 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -155,18 +155,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
+	.. include:: common_options.rst
 
 	-f, --bpffs
 		  Show file names of pinned maps.
@@ -175,10 +164,6 @@ OPTIONS
 		  Do not automatically attempt to mount any virtual file system
 		  (such as tracefs or BPF virtual file system) when necessary.
 
-	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
-
 EXAMPLES
 ========
 **# bpftool map show**
diff --git a/tools/bpf/bpftool/Documentation/bpftool-net.rst b/tools/bpf/bpftool/Documentation/bpftool-net.rst
index aa7450736179..56439c32934d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
@@ -75,22 +75,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
-
-	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
+	.. include:: common_options.rst
 
 EXAMPLES
 ========
diff --git a/tools/bpf/bpftool/Documentation/bpftool-perf.rst b/tools/bpf/bpftool/Documentation/bpftool-perf.rst
index 9c592b7c6775..36d257a36e9b 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-perf.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-perf.rst
@@ -40,22 +40,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
-
-	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
+	.. include:: common_options.rst
 
 EXAMPLES
 ========
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 82e356b664e8..9b2b18e2a3ac 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -210,18 +210,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
+	.. include:: common_options.rst
 
 	-f, --bpffs
 		  When showing BPF programs, show file names of pinned
@@ -234,11 +223,6 @@ OPTIONS
 		  Do not automatically attempt to mount any virtual file system
 		  (such as tracefs or BPF virtual file system) when necessary.
 
-	-d, --debug
-		  Print all logs available, even debug-level information. This
-		  includes logs from libbpf as well as from the verifier, when
-		  attempting to load programs.
-
 EXAMPLES
 ========
 **# bpftool prog show**
diff --git a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
index d93cd1cb8b0f..315f1f21f2ba 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
@@ -60,23 +60,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short generic help message (similar to **bpftool help**).
-
-	-V, --version
-		  Print version number (similar to **bpftool version**).
-
-	-j, --json
-		  Generate JSON output. For commands that cannot produce JSON, this
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
-
-	-d, --debug
-		  Print all logs available, even debug-level information. This
-		  includes logs from libbpf as well as from the verifier, when
-		  attempting to load programs.
+	.. include:: common_options.rst
 
 EXAMPLES
 ========
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index a3629a3f1175..b87f8c2df49d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -46,24 +46,7 @@ DESCRIPTION
 
 OPTIONS
 =======
-	-h, --help
-		  Print short help message (similar to **bpftool help**).
-
-	-V, --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
-		  option has no effect.
-
-	-p, --pretty
-		  Generate human-readable JSON output. Implies **-j**.
+	.. include:: common_options.rst
 
 	-m, --mapcompat
 		  Allow loading maps with unknown map definitions.
@@ -72,11 +55,6 @@ OPTIONS
 		  Do not automatically attempt to mount any virtual file system
 		  (such as tracefs or BPF virtual file system) when necessary.
 
-	-d, --debug
-		  Print all logs available, even debug-level information. This
-		  includes logs from libbpf as well as from the verifier, when
-		  attempting to load programs.
-
 SEE ALSO
 ========
 	**bpf**\ (2),
diff --git a/tools/bpf/bpftool/Documentation/common_options.rst b/tools/bpf/bpftool/Documentation/common_options.rst
new file mode 100644
index 000000000000..05d06c74dcaa
--- /dev/null
+++ b/tools/bpf/bpftool/Documentation/common_options.rst
@@ -0,0 +1,22 @@
+-h, --help
+	  Print short help message (similar to **bpftool help**).
+
+-V, --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
+	  option has no effect.
+
+-p, --pretty
+	  Generate human-readable JSON output. Implies **-j**.
+
+-d, --debug
+	  Print all logs available, even debug-level information. This includes
+	  logs from libbpf as well as from the verifier, when attempting to
+	  load programs.
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 2/3] tools: bpftool: include common options from separate file
  2020-09-04 20:56 ` [PATCH bpf-next 2/3] tools: bpftool: include common options from separate file Quentin Monnet
@ 2020-09-04 21:46   ` Andrii Nakryiko
  0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-09-04 21:46 UTC (permalink / raw)
  To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking

On Fri, Sep 4, 2020 at 1:57 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Nearly all man pages for bpftool have the same common set of option
> flags (--help, --version, --json, --pretty, --debug). The description is
> duplicated across all the pages, which is more difficult to maintain if
> the description of an option changes. It may also be confusing to sort
> out what options are not "common" and should not be copied when creating
> new manual pages.
>
> Let's move the description for those common options to a separate file,
> which is included with a RST directive when generating the man pages.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/bpf/bpftool/Documentation/Makefile      |  2 +-
>  .../bpf/bpftool/Documentation/bpftool-btf.rst | 17 +------------
>  .../bpftool/Documentation/bpftool-cgroup.rst  | 17 +------------
>  .../bpftool/Documentation/bpftool-feature.rst | 17 +------------
>  .../bpf/bpftool/Documentation/bpftool-gen.rst | 17 +------------
>  .../bpftool/Documentation/bpftool-iter.rst    | 11 +--------
>  .../bpftool/Documentation/bpftool-link.rst    | 17 +------------
>  .../bpf/bpftool/Documentation/bpftool-map.rst | 17 +------------
>  .../bpf/bpftool/Documentation/bpftool-net.rst | 17 +------------
>  .../bpftool/Documentation/bpftool-perf.rst    | 17 +------------
>  .../bpftool/Documentation/bpftool-prog.rst    | 18 +-------------
>  .../Documentation/bpftool-struct_ops.rst      | 18 +-------------
>  tools/bpf/bpftool/Documentation/bpftool.rst   | 24 +------------------
>  .../bpftool/Documentation/common_options.rst  | 22 +++++++++++++++++
>  14 files changed, 35 insertions(+), 196 deletions(-)
>  create mode 100644 tools/bpf/bpftool/Documentation/common_options.rst
>

[...]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH bpf-next 3/3] tools: bpftool: automate generation for "SEE ALSO" sections in man pages
  2020-09-04 20:56 [PATCH bpf-next 0/3] tools: bpftool: print built-in features, automate some of the documentation Quentin Monnet
  2020-09-04 20:56 ` [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version Quentin Monnet
  2020-09-04 20:56 ` [PATCH bpf-next 2/3] tools: bpftool: include common options from separate file Quentin Monnet
@ 2020-09-04 20:56 ` Quentin Monnet
  2020-09-04 21:51   ` Andrii Nakryiko
  2 siblings, 1 reply; 11+ messages in thread
From: Quentin Monnet @ 2020-09-04 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann; +Cc: bpf, netdev, Quentin Monnet

The "SEE ALSO" sections of bpftool's manual pages refer to bpf(2),
bpf-helpers(7), then all existing bpftool man pages (save the current
one).

This leads to nearly-identical lists being duplicated in all manual
pages. Ideally, when a new page is created, all lists should be updated
accordingly, but this has led to omissions and inconsistencies multiple
times in the past.

Let's take it out of the RST files and generate the "SEE ALSO" sections
automatically in the Makefile when generating the man pages. The lists
are not really useful in the RST anyway because all other pages are
available in the same directory.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/Documentation/Makefile        | 12 +++++++++++-
 tools/bpf/bpftool/Documentation/bpftool-btf.rst | 17 -----------------
 .../bpftool/Documentation/bpftool-cgroup.rst    | 16 ----------------
 .../bpftool/Documentation/bpftool-feature.rst   | 16 ----------------
 tools/bpf/bpftool/Documentation/bpftool-gen.rst | 16 ----------------
 .../bpf/bpftool/Documentation/bpftool-iter.rst  | 16 ----------------
 .../bpf/bpftool/Documentation/bpftool-link.rst  | 17 -----------------
 tools/bpf/bpftool/Documentation/bpftool-map.rst | 16 ----------------
 tools/bpf/bpftool/Documentation/bpftool-net.rst | 17 -----------------
 .../bpf/bpftool/Documentation/bpftool-perf.rst  | 17 -----------------
 .../bpf/bpftool/Documentation/bpftool-prog.rst  | 16 ----------------
 .../Documentation/bpftool-struct_ops.rst        | 17 -----------------
 tools/bpf/bpftool/Documentation/bpftool.rst     | 16 ----------------
 13 files changed, 11 insertions(+), 198 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/Makefile b/tools/bpf/bpftool/Documentation/Makefile
index becbb8c52257..86233619215c 100644
--- a/tools/bpf/bpftool/Documentation/Makefile
+++ b/tools/bpf/bpftool/Documentation/Makefile
@@ -29,11 +29,21 @@ man8: $(DOC_MAN8)
 
 RST2MAN_DEP := $(shell command -v rst2man 2>/dev/null)
 
+list_pages = $(sort $(basename $(filter-out $(1),$(MAN8_RST))))
+see_also = $(subst " ",, \
+	"\n" \
+	"SEE ALSO\n" \
+	"========\n" \
+	"\t**bpf**\ (2),\n" \
+	"\t**bpf-helpers**\\ (7)" \
+	$(foreach page,$(call list_pages,$(1)),",\n\t**$(page)**\\ (8)") \
+	"\n")
+
 $(OUTPUT)%.8: %.rst
 ifndef RST2MAN_DEP
 	$(error "rst2man not found, but required to generate man pages")
 endif
-	$(QUIET_GEN)rst2man $< > $@
+	$(QUIET_GEN)( cat $< ; printf $(call see_also,$<) ) | rst2man > $@
 
 clean: helpers-clean
 	$(call QUIET_CLEAN, Documentation)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
index 0020bb55cf7e..b3e909ef6791 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
@@ -214,20 +214,3 @@ All the standard ways to specify map or program are supported:
 **# bpftool btf dump prog tag b88e0a09b1d9759d**
 
 **# bpftool btf dump prog pinned /sys/fs/bpf/prog_name**
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index 3dba89db000e..790944c35602 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -143,19 +143,3 @@ EXAMPLES
 ::
 
     ID       AttachType      AttachFlags     Name
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-feature.rst b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
index f1aae5690e3c..dd3771bdbc57 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-feature.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
@@ -72,19 +72,3 @@ DESCRIPTION
 OPTIONS
 =======
 	.. include:: common_options.rst
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
index e3b7ff3c09d7..8b4a18463d55 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-gen.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
@@ -275,19 +275,3 @@ and global variables.
   my_static_var: 7
 
 This is a stripped-out version of skeleton generated for above example code.
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-iter.rst b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
index b688cf11805c..51f49bead619 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-iter.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
@@ -68,19 +68,3 @@ EXAMPLES
 
    Create a file-based bpf iterator from bpf_iter_hashmap.o and map with
    id 20, and pin it to /sys/fs/bpf/my_hashmap
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-link.rst b/tools/bpf/bpftool/Documentation/bpftool-link.rst
index 001689efc7fa..8e4e28c1b78a 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-link.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-link.rst
@@ -106,20 +106,3 @@ EXAMPLES
 ::
 
     -rw------- 1 root root 0 Apr 23 21:39 link
-
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index e06a65cd467e..4fe8632bb3a3 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -261,19 +261,3 @@ would be lost as soon as bpftool exits).
 
   key: 00 00 00 00  value: 22 02 00 00
   Found 1 element
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-net.rst b/tools/bpf/bpftool/Documentation/bpftool-net.rst
index 56439c32934d..d8165d530937 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
@@ -172,20 +172,3 @@ EXAMPLES
 ::
 
       xdp:
-
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-perf.rst b/tools/bpf/bpftool/Documentation/bpftool-perf.rst
index 36d257a36e9b..e958ce91de72 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-perf.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-perf.rst
@@ -63,20 +63,3 @@ EXAMPLES
      {"pid":21765,"fd":5,"prog_id":7,"fd_type":"kretprobe","func":"__x64_sys_nanosleep","offset":0}, \
      {"pid":21767,"fd":5,"prog_id":8,"fd_type":"tracepoint","tracepoint":"sys_enter_nanosleep"}, \
      {"pid":21800,"fd":5,"prog_id":9,"fd_type":"uprobe","filename":"/home/yhs/a.out","offset":1159}]
-
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 9b2b18e2a3ac..358c7309d419 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -326,19 +326,3 @@ EXAMPLES
       40176203 cycles                                                 (83.05%)
       42518139 instructions    #   1.06 insns per cycle               (83.39%)
            123 llc_misses      #   2.89 LLC misses per million insns  (83.15%)
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-struct_ops**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
index 315f1f21f2ba..506e70ee78e9 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
@@ -82,20 +82,3 @@ EXAMPLES
 ::
 
    Registered tcp_congestion_ops cubic id 110
-
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool**\ (8),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index b87f8c2df49d..e7d949334961 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -54,19 +54,3 @@ OPTIONS
 	-n, --nomount
 		  Do not automatically attempt to mount any virtual file system
 		  (such as tracefs or BPF virtual file system) when necessary.
-
-SEE ALSO
-========
-	**bpf**\ (2),
-	**bpf-helpers**\ (7),
-	**bpftool-btf**\ (8),
-	**bpftool-cgroup**\ (8),
-	**bpftool-feature**\ (8),
-	**bpftool-gen**\ (8),
-	**bpftool-iter**\ (8),
-	**bpftool-link**\ (8),
-	**bpftool-map**\ (8),
-	**bpftool-net**\ (8),
-	**bpftool-perf**\ (8),
-	**bpftool-prog**\ (8),
-	**bpftool-struct_ops**\ (8)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 3/3] tools: bpftool: automate generation for "SEE ALSO" sections in man pages
  2020-09-04 20:56 ` [PATCH bpf-next 3/3] tools: bpftool: automate generation for "SEE ALSO" sections in man pages Quentin Monnet
@ 2020-09-04 21:51   ` Andrii Nakryiko
  2020-09-07 14:50     ` Quentin Monnet
  0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-09-04 21:51 UTC (permalink / raw)
  To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking

On Fri, Sep 4, 2020 at 1:58 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> The "SEE ALSO" sections of bpftool's manual pages refer to bpf(2),
> bpf-helpers(7), then all existing bpftool man pages (save the current
> one).
>
> This leads to nearly-identical lists being duplicated in all manual
> pages. Ideally, when a new page is created, all lists should be updated
> accordingly, but this has led to omissions and inconsistencies multiple
> times in the past.
>
> Let's take it out of the RST files and generate the "SEE ALSO" sections
> automatically in the Makefile when generating the man pages. The lists
> are not really useful in the RST anyway because all other pages are
> available in the same directory.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

but see note about printf and format string below

>  tools/bpf/bpftool/Documentation/Makefile        | 12 +++++++++++-
>  tools/bpf/bpftool/Documentation/bpftool-btf.rst | 17 -----------------
>  .../bpftool/Documentation/bpftool-cgroup.rst    | 16 ----------------
>  .../bpftool/Documentation/bpftool-feature.rst   | 16 ----------------
>  tools/bpf/bpftool/Documentation/bpftool-gen.rst | 16 ----------------
>  .../bpf/bpftool/Documentation/bpftool-iter.rst  | 16 ----------------
>  .../bpf/bpftool/Documentation/bpftool-link.rst  | 17 -----------------
>  tools/bpf/bpftool/Documentation/bpftool-map.rst | 16 ----------------
>  tools/bpf/bpftool/Documentation/bpftool-net.rst | 17 -----------------
>  .../bpf/bpftool/Documentation/bpftool-perf.rst  | 17 -----------------
>  .../bpf/bpftool/Documentation/bpftool-prog.rst  | 16 ----------------
>  .../Documentation/bpftool-struct_ops.rst        | 17 -----------------
>  tools/bpf/bpftool/Documentation/bpftool.rst     | 16 ----------------
>  13 files changed, 11 insertions(+), 198 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/Makefile b/tools/bpf/bpftool/Documentation/Makefile
> index becbb8c52257..86233619215c 100644
> --- a/tools/bpf/bpftool/Documentation/Makefile
> +++ b/tools/bpf/bpftool/Documentation/Makefile
> @@ -29,11 +29,21 @@ man8: $(DOC_MAN8)
>
>  RST2MAN_DEP := $(shell command -v rst2man 2>/dev/null)
>
> +list_pages = $(sort $(basename $(filter-out $(1),$(MAN8_RST))))
> +see_also = $(subst " ",, \
> +       "\n" \
> +       "SEE ALSO\n" \
> +       "========\n" \
> +       "\t**bpf**\ (2),\n" \
> +       "\t**bpf-helpers**\\ (7)" \
> +       $(foreach page,$(call list_pages,$(1)),",\n\t**$(page)**\\ (8)") \
> +       "\n")
> +
>  $(OUTPUT)%.8: %.rst
>  ifndef RST2MAN_DEP
>         $(error "rst2man not found, but required to generate man pages")
>  endif
> -       $(QUIET_GEN)rst2man $< > $@
> +       $(QUIET_GEN)( cat $< ; printf $(call see_also,$<) ) | rst2man > $@

a bit dangerous to pass string directly as a format string due to %
interpretation. Did you try echo -e "...\n..." ?

>
>  clean: helpers-clean
>         $(call QUIET_CLEAN, Documentation)
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> index 0020bb55cf7e..b3e909ef6791 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> @@ -214,20 +214,3 @@ All the standard ways to specify map or program are supported:
>  **# bpftool btf dump prog tag b88e0a09b1d9759d**
>
>  **# bpftool btf dump prog pinned /sys/fs/bpf/prog_name**

[...]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 3/3] tools: bpftool: automate generation for "SEE ALSO" sections in man pages
  2020-09-04 21:51   ` Andrii Nakryiko
@ 2020-09-07 14:50     ` Quentin Monnet
  0 siblings, 0 replies; 11+ messages in thread
From: Quentin Monnet @ 2020-09-07 14:50 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking

On 04/09/2020 22:51, Andrii Nakryiko wrote:
> On Fri, Sep 4, 2020 at 1:58 PM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> The "SEE ALSO" sections of bpftool's manual pages refer to bpf(2),
>> bpf-helpers(7), then all existing bpftool man pages (save the current
>> one).
>>
>> This leads to nearly-identical lists being duplicated in all manual
>> pages. Ideally, when a new page is created, all lists should be updated
>> accordingly, but this has led to omissions and inconsistencies multiple
>> times in the past.
>>
>> Let's take it out of the RST files and generate the "SEE ALSO" sections
>> automatically in the Makefile when generating the man pages. The lists
>> are not really useful in the RST anyway because all other pages are
>> available in the same directory.
>>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> ---
> 
> Acked-by: Andrii Nakryiko <andriin@fb.com>
> 
> but see note about printf and format string below

Thanks!

>> diff --git a/tools/bpf/bpftool/Documentation/Makefile b/tools/bpf/bpftool/Documentation/Makefile
>> index becbb8c52257..86233619215c 100644
>> --- a/tools/bpf/bpftool/Documentation/Makefile
>> +++ b/tools/bpf/bpftool/Documentation/Makefile
>> @@ -29,11 +29,21 @@ man8: $(DOC_MAN8)
>>
>>  RST2MAN_DEP := $(shell command -v rst2man 2>/dev/null)
>>
>> +list_pages = $(sort $(basename $(filter-out $(1),$(MAN8_RST))))
>> +see_also = $(subst " ",, \
>> +       "\n" \
>> +       "SEE ALSO\n" \
>> +       "========\n" \
>> +       "\t**bpf**\ (2),\n" \
>> +       "\t**bpf-helpers**\\ (7)" \
>> +       $(foreach page,$(call list_pages,$(1)),",\n\t**$(page)**\\ (8)") \
>> +       "\n")
>> +
>>  $(OUTPUT)%.8: %.rst
>>  ifndef RST2MAN_DEP
>>         $(error "rst2man not found, but required to generate man pages")
>>  endif
>> -       $(QUIET_GEN)rst2man $< > $@
>> +       $(QUIET_GEN)( cat $< ; printf $(call see_also,$<) ) | rst2man > $@
> 
> a bit dangerous to pass string directly as a format string due to %
> interpretation. Did you try echo -e "...\n..." ?

I believe printf is supposed to be more portable, this is why I used it.
It seems unlikely we end up with a percent sign in a bpftool man page
name, but I can switch to "echo -e" for v2.

Quentin

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-09-09  8:35 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 20:56 [PATCH bpf-next 0/3] tools: bpftool: print built-in features, automate some of the documentation Quentin Monnet
2020-09-04 20:56 ` [PATCH bpf-next 1/3] tools: bpftool: print optional built-in features along with version Quentin Monnet
2020-09-04 21:45   ` Andrii Nakryiko
2020-09-07 14:50     ` Quentin Monnet
2020-09-08 23:20       ` Andrii Nakryiko
2020-09-09  8:35         ` Quentin Monnet
2020-09-04 20:56 ` [PATCH bpf-next 2/3] tools: bpftool: include common options from separate file Quentin Monnet
2020-09-04 21:46   ` Andrii Nakryiko
2020-09-04 20:56 ` [PATCH bpf-next 3/3] tools: bpftool: automate generation for "SEE ALSO" sections in man pages Quentin Monnet
2020-09-04 21:51   ` Andrii Nakryiko
2020-09-07 14:50     ` Quentin Monnet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).