[PATCH bpf-next v2 0/3] tools: bpftool: add an option for debug output from libbpf and verifier

[PATCH bpf-next v2 0/3] tools: bpftool: add an option for debug output from libbpf and verifier

[Quentin Monnet]

Hi,
This series adds an option to bpftool to make it print additional
information via libbpf and the kernel verifier when attempting to load
programs.

A new API function is added to libbpf in order to pass the log_level from
bpftool with the bpf_object__* part of the API.

Options for a finer control over the log levels to use for libbpf and the
verifier could be added in the future, if desired.

v2:
- Do not add distinct options for libbpf and verifier logs, just keep the
  one that sets all log levels to their maximum. Rename the option.
- Do not offer a way to pick desired log levels. The choice is "use the
  option to print all logs" or "stick with the defaults".
- Do not export BPF_LOG_* flags to user header.
- Update all man pages (most bpftool operations use libbpf and may print
  libbpf logs). Verifier logs are only used when attempting to load
  programs for now, so bpftool-prog.rst and bpftool.rst remain the only
  pages updated in that regard.

Previous discussion available at:
https://lore.kernel.org/bpf/20190429095227.9745-1-quentin.monnet@netronome.com/

Quentin Monnet (3):
  tools: bpftool: add -d option to get debug output from libbpf
  libbpf: add bpf_object__load_xattr() API function to pass log_level
  tools: bpftool: make -d option print debug output from verifier

 .../bpf/bpftool/Documentation/bpftool-btf.rst |  4 +++
 .../bpftool/Documentation/bpftool-cgroup.rst  |  4 +++
 .../bpftool/Documentation/bpftool-feature.rst |  4 +++
 .../bpf/bpftool/Documentation/bpftool-map.rst |  4 +++
 .../bpf/bpftool/Documentation/bpftool-net.rst |  4 +++
 .../bpftool/Documentation/bpftool-perf.rst    |  4 +++
 .../bpftool/Documentation/bpftool-prog.rst    |  5 ++++
 tools/bpf/bpftool/Documentation/bpftool.rst   |  4 +++
 tools/bpf/bpftool/bash-completion/bpftool     |  2 +-
 tools/bpf/bpftool/main.c                      | 16 ++++++++++-
 tools/bpf/bpftool/main.h                      |  1 +
 tools/bpf/bpftool/prog.c                      | 27 ++++++++++++-------
 tools/lib/bpf/Makefile                        |  2 +-
 tools/lib/bpf/libbpf.c                        | 20 +++++++++++---
 tools/lib/bpf/libbpf.h                        |  6 +++++
 tools/lib/bpf/libbpf.map                      |  5 ++++
 16 files changed, 96 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH bpf-next v2 1/3] tools: bpftool: add -d option to get debug output from libbpf

[Quentin Monnet]

libbpf has three levels of priority for output messages: warn, info,
debug. By default, debug output is not printed to the console.

Add a new "--debug" (short name: "-d") option to bpftool to print libbpf
logs for all three levels.

Internally, we simply use the function provided by libbpf to replace the
default printing function by one that prints logs regardless of their
level.

v2:
- Remove the possibility to select the log-levels to use (v1 offered a
  combination of "warn", "info" and "debug").
- Rename option and offer a short name: -d|--debug.
- Add option description to all bpftool manual pages (instead of
  bpftool-prog.rst only), as all commands use libbpf.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/Documentation/bpftool-btf.rst    |  4 ++++
 tools/bpf/bpftool/Documentation/bpftool-cgroup.rst |  4 ++++
 .../bpf/bpftool/Documentation/bpftool-feature.rst  |  4 ++++
 tools/bpf/bpftool/Documentation/bpftool-map.rst    |  4 ++++
 tools/bpf/bpftool/Documentation/bpftool-net.rst    |  4 ++++
 tools/bpf/bpftool/Documentation/bpftool-perf.rst   |  4 ++++
 tools/bpf/bpftool/Documentation/bpftool-prog.rst   |  4 ++++
 tools/bpf/bpftool/Documentation/bpftool.rst        |  3 +++
 tools/bpf/bpftool/bash-completion/bpftool          |  2 +-
 tools/bpf/bpftool/main.c                           | 14 +++++++++++++-
 10 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
index 2dbc1413fabd..00668df1bf7a 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
@@ -67,6 +67,10 @@ OPTIONS
 	-p, --pretty
 		  Generate human-readable JSON output. Implies **-j**.
 
+	-d, --debug
+		  Print all logs available from libbpf, including debug-level
+		  information.
+
 EXAMPLES
 ========
 **# bpftool btf dump id 1226**
diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index ac26876389c2..36807735e2a5 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -113,6 +113,10 @@ OPTIONS
 	-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 14180e887082..4d08f35034a2 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-feature.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
@@ -73,6 +73,10 @@ OPTIONS
 	-p, --pretty
 		  Generate human-readable JSON output. Implies **-j**.
 
+	-d, --debug
+		  Print all logs available from libbpf, including debug-level
+		  information.
+
 SEE ALSO
 ========
 	**bpf**\ (2),
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 13ef27b39f20..490b4501cb6e 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -152,6 +152,10 @@ 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 934580850f42..d8e5237a2085 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
@@ -65,6 +65,10 @@ OPTIONS
 	-p, --pretty
 		  Generate human-readable JSON output. Implies **-j**.
 
+	-d, --debug
+		  Print all logs available from libbpf, including debug-level
+		  information.
+
 EXAMPLES
 ========
 
diff --git a/tools/bpf/bpftool/Documentation/bpftool-perf.rst b/tools/bpf/bpftool/Documentation/bpftool-perf.rst
index 0c7576523a21..e252bd0bc434 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-perf.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-perf.rst
@@ -53,6 +53,10 @@ OPTIONS
 	-p, --pretty
 		  Generate human-readable JSON output. Implies **-j**.
 
+	-d, --debug
+		  Print all logs available from libbpf, including debug-level
+		  information.
+
 EXAMPLES
 ========
 
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index e8118544d118..9a92614569e6 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -174,6 +174,10 @@ 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 prog show**
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index 3e562d7fd56f..43dba0717953 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -66,6 +66,9 @@ 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.
 
 SEE ALSO
 ========
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 50e402a5a9c8..3a476e25d046 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -181,7 +181,7 @@ _bpftool()
 
     # Deal with options
     if [[ ${words[cword]} == -* ]]; then
-        local c='--version --json --pretty --bpffs --mapcompat'
+        local c='--version --json --pretty --bpffs --mapcompat --debug'
         COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
         return 0
     fi
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 1ac1fc520e6a..d74293938a05 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -10,6 +10,7 @@
 #include <string.h>
 
 #include <bpf.h>
+#include <libbpf.h>
 
 #include "main.h"
 
@@ -77,6 +78,13 @@ static int do_version(int argc, char **argv)
 	return 0;
 }
 
+static int __printf(2, 0)
+print_all_levels(__maybe_unused enum libbpf_print_level level,
+		 const char *format, va_list args)
+{
+	return vfprintf(stderr, format, args);
+}
+
 int cmd_select(const struct cmd *cmds, int argc, char **argv,
 	       int (*help)(int argc, char **argv))
 {
@@ -317,6 +325,7 @@ int main(int argc, char **argv)
 		{ "bpffs",	no_argument,	NULL,	'f' },
 		{ "mapcompat",	no_argument,	NULL,	'm' },
 		{ "nomount",	no_argument,	NULL,	'n' },
+		{ "debug",	no_argument,	NULL,	'd' },
 		{ 0 }
 	};
 	int opt, ret;
@@ -332,7 +341,7 @@ int main(int argc, char **argv)
 	hash_init(map_table.table);
 
 	opterr = 0;
-	while ((opt = getopt_long(argc, argv, "Vhpjfmn",
+	while ((opt = getopt_long(argc, argv, "Vhpjfmnd",
 				  options, NULL)) >= 0) {
 		switch (opt) {
 		case 'V':
@@ -362,6 +371,9 @@ int main(int argc, char **argv)
 		case 'n':
 			block_mount = true;
 			break;
+		case 'd':
+			libbpf_set_print(print_all_levels);
+			break;
 		default:
 			p_err("unrecognized option '%s'", argv[optind - 1]);
 			if (json_output)
-- 
2.17.1

Re: [PATCH bpf-next v2 1/3] tools: bpftool: add -d option to get debug output from libbpf

[Andrii Nakryiko]

On Thu, May 23, 2019 at 3:54 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> libbpf has three levels of priority for output messages: warn, info,
> debug. By default, debug output is not printed to the console.
>
> Add a new "--debug" (short name: "-d") option to bpftool to print libbpf
> logs for all three levels.
>
> Internally, we simply use the function provided by libbpf to replace the
> default printing function by one that prints logs regardless of their
> level.
>
> v2:
> - Remove the possibility to select the log-levels to use (v1 offered a
>   combination of "warn", "info" and "debug").
> - Rename option and offer a short name: -d|--debug.

Such and option in CLI tools is usually called -v|--verbose, I'm
wondering if it might be a better name choice?

Btw, some tools also use -v, -vv and -vvv to define different levels
of verbosity, which is something we can consider in the future, as
it's backwards compatible.

> - Add option description to all bpftool manual pages (instead of
>   bpftool-prog.rst only), as all commands use libbpf.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  tools/bpf/bpftool/Documentation/bpftool-btf.rst    |  4 ++++
>  tools/bpf/bpftool/Documentation/bpftool-cgroup.rst |  4 ++++
>  .../bpf/bpftool/Documentation/bpftool-feature.rst  |  4 ++++
>  tools/bpf/bpftool/Documentation/bpftool-map.rst    |  4 ++++
>  tools/bpf/bpftool/Documentation/bpftool-net.rst    |  4 ++++
>  tools/bpf/bpftool/Documentation/bpftool-perf.rst   |  4 ++++
>  tools/bpf/bpftool/Documentation/bpftool-prog.rst   |  4 ++++
>  tools/bpf/bpftool/Documentation/bpftool.rst        |  3 +++
>  tools/bpf/bpftool/bash-completion/bpftool          |  2 +-
>  tools/bpf/bpftool/main.c                           | 14 +++++++++++++-
>  10 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> index 2dbc1413fabd..00668df1bf7a 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> @@ -67,6 +67,10 @@ OPTIONS
>         -p, --pretty
>                   Generate human-readable JSON output. Implies **-j**.
>
> +       -d, --debug
> +                 Print all logs available from libbpf, including debug-level
> +                 information.
> +
>  EXAMPLES
>  ========
>  **# bpftool btf dump id 1226**
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> index ac26876389c2..36807735e2a5 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> @@ -113,6 +113,10 @@ OPTIONS
>         -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 14180e887082..4d08f35034a2 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-feature.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
> @@ -73,6 +73,10 @@ OPTIONS
>         -p, --pretty
>                   Generate human-readable JSON output. Implies **-j**.
>
> +       -d, --debug
> +                 Print all logs available from libbpf, including debug-level
> +                 information.
> +
>  SEE ALSO
>  ========
>         **bpf**\ (2),
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> index 13ef27b39f20..490b4501cb6e 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> @@ -152,6 +152,10 @@ 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 934580850f42..d8e5237a2085 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> @@ -65,6 +65,10 @@ OPTIONS
>         -p, --pretty
>                   Generate human-readable JSON output. Implies **-j**.
>
> +       -d, --debug
> +                 Print all logs available from libbpf, including debug-level
> +                 information.
> +
>  EXAMPLES
>  ========
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-perf.rst b/tools/bpf/bpftool/Documentation/bpftool-perf.rst
> index 0c7576523a21..e252bd0bc434 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-perf.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-perf.rst
> @@ -53,6 +53,10 @@ OPTIONS
>         -p, --pretty
>                   Generate human-readable JSON output. Implies **-j**.
>
> +       -d, --debug
> +                 Print all logs available from libbpf, including debug-level
> +                 information.
> +
>  EXAMPLES
>  ========
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index e8118544d118..9a92614569e6 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -174,6 +174,10 @@ 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 prog show**
> diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
> index 3e562d7fd56f..43dba0717953 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
> @@ -66,6 +66,9 @@ 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.
>
>  SEE ALSO
>  ========
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 50e402a5a9c8..3a476e25d046 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -181,7 +181,7 @@ _bpftool()
>
>      # Deal with options
>      if [[ ${words[cword]} == -* ]]; then
> -        local c='--version --json --pretty --bpffs --mapcompat'
> +        local c='--version --json --pretty --bpffs --mapcompat --debug'
>          COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
>          return 0
>      fi
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 1ac1fc520e6a..d74293938a05 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -10,6 +10,7 @@
>  #include <string.h>
>
>  #include <bpf.h>
> +#include <libbpf.h>
>
>  #include "main.h"
>
> @@ -77,6 +78,13 @@ static int do_version(int argc, char **argv)
>         return 0;
>  }
>
> +static int __printf(2, 0)
> +print_all_levels(__maybe_unused enum libbpf_print_level level,
> +                const char *format, va_list args)
> +{
> +       return vfprintf(stderr, format, args);
> +}
> +
>  int cmd_select(const struct cmd *cmds, int argc, char **argv,
>                int (*help)(int argc, char **argv))
>  {
> @@ -317,6 +325,7 @@ int main(int argc, char **argv)
>                 { "bpffs",      no_argument,    NULL,   'f' },
>                 { "mapcompat",  no_argument,    NULL,   'm' },
>                 { "nomount",    no_argument,    NULL,   'n' },
> +               { "debug",      no_argument,    NULL,   'd' },
>                 { 0 }
>         };
>         int opt, ret;
> @@ -332,7 +341,7 @@ int main(int argc, char **argv)
>         hash_init(map_table.table);
>
>         opterr = 0;
> -       while ((opt = getopt_long(argc, argv, "Vhpjfmn",
> +       while ((opt = getopt_long(argc, argv, "Vhpjfmnd",
>                                   options, NULL)) >= 0) {
>                 switch (opt) {
>                 case 'V':
> @@ -362,6 +371,9 @@ int main(int argc, char **argv)
>                 case 'n':
>                         block_mount = true;
>                         break;
> +               case 'd':
> +                       libbpf_set_print(print_all_levels);
> +                       break;
>                 default:
>                         p_err("unrecognized option '%s'", argv[optind - 1]);
>                         if (json_output)
> --
> 2.17.1
>

Re: [PATCH bpf-next v2 1/3] tools: bpftool: add -d option to get debug output from libbpf

[Jakub Kicinski]

On Thu, 23 May 2019 09:20:52 -0700, Andrii Nakryiko wrote:
> On Thu, May 23, 2019 at 3:54 AM Quentin Monnet wrote:
> >
> > libbpf has three levels of priority for output messages: warn, info,
> > debug. By default, debug output is not printed to the console.
> >
> > Add a new "--debug" (short name: "-d") option to bpftool to print libbpf
> > logs for all three levels.
> >
> > Internally, we simply use the function provided by libbpf to replace the
> > default printing function by one that prints logs regardless of their
> > level.
> >
> > v2:
> > - Remove the possibility to select the log-levels to use (v1 offered a
> >   combination of "warn", "info" and "debug").
> > - Rename option and offer a short name: -d|--debug.  
> 
> Such and option in CLI tools is usually called -v|--verbose, I'm
> wondering if it might be a better name choice?
> 
> Btw, some tools also use -v, -vv and -vvv to define different levels
> of verbosity, which is something we can consider in the future, as
> it's backwards compatible.

That was my weak suggestion.  Sometimes -v is used for version, e.g.
GCC.  -d is sometimes used for debug, e.g. man, iproute2 uses it as
short for "detailed".  If the consensus is that -v is better I don't
really mind.

Re: [PATCH bpf-next v2 1/3] tools: bpftool: add -d option to get debug output from libbpf

[Andrii Nakryiko]

On Thu, May 23, 2019 at 1:44 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 23 May 2019 09:20:52 -0700, Andrii Nakryiko wrote:
> > On Thu, May 23, 2019 at 3:54 AM Quentin Monnet wrote:
> > >
> > > libbpf has three levels of priority for output messages: warn, info,
> > > debug. By default, debug output is not printed to the console.
> > >
> > > Add a new "--debug" (short name: "-d") option to bpftool to print libbpf
> > > logs for all three levels.
> > >
> > > Internally, we simply use the function provided by libbpf to replace the
> > > default printing function by one that prints logs regardless of their
> > > level.
> > >
> > > v2:
> > > - Remove the possibility to select the log-levels to use (v1 offered a
> > >   combination of "warn", "info" and "debug").
> > > - Rename option and offer a short name: -d|--debug.
> >
> > Such and option in CLI tools is usually called -v|--verbose, I'm
> > wondering if it might be a better name choice?
> >
> > Btw, some tools also use -v, -vv and -vvv to define different levels
> > of verbosity, which is something we can consider in the future, as
> > it's backwards compatible.
>
> That was my weak suggestion.  Sometimes -v is used for version, e.g.
> GCC.  -d is sometimes used for debug, e.g. man, iproute2 uses it as
> short for "detailed".  If the consensus is that -v is better I don't
> really mind.

It's minor, so I'm not insisting at all, just wasn't sure it was
brought up. bpftool is sufficiently different in its conventions from
other modern CLIs anyways.

As for -v for version. It seems like the trend is to use -V|--version
for version, and -v|--verbose for verbosity. I've also seen some tools
option for `cli version` (subcommand) for version. Anyway, no strong
preferences from me either.

Re: [PATCH bpf-next v2 1/3] tools: bpftool: add -d option to get debug output from libbpf

[Quentin Monnet]

2019-05-23 13:57 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Thu, May 23, 2019 at 1:44 PM Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
>>
>> On Thu, 23 May 2019 09:20:52 -0700, Andrii Nakryiko wrote:
>>> On Thu, May 23, 2019 at 3:54 AM Quentin Monnet wrote:
>>>>
>>>> libbpf has three levels of priority for output messages: warn, info,
>>>> debug. By default, debug output is not printed to the console.
>>>>
>>>> Add a new "--debug" (short name: "-d") option to bpftool to print libbpf
>>>> logs for all three levels.
>>>>
>>>> Internally, we simply use the function provided by libbpf to replace the
>>>> default printing function by one that prints logs regardless of their
>>>> level.
>>>>
>>>> v2:
>>>> - Remove the possibility to select the log-levels to use (v1 offered a
>>>>   combination of "warn", "info" and "debug").
>>>> - Rename option and offer a short name: -d|--debug.
>>>
>>> Such and option in CLI tools is usually called -v|--verbose, I'm
>>> wondering if it might be a better name choice?
>>>
>>> Btw, some tools also use -v, -vv and -vvv to define different levels
>>> of verbosity, which is something we can consider in the future, as
>>> it's backwards compatible.
>>
>> That was my weak suggestion.  Sometimes -v is used for version, e.g.
>> GCC.  -d is sometimes used for debug, e.g. man, iproute2 uses it as
>> short for "detailed".  If the consensus is that -v is better I don't
>> really mind.
> 
> It's minor, so I'm not insisting at all, just wasn't sure it was
> brought up. bpftool is sufficiently different in its conventions from
> other modern CLIs anyways.
> 
> As for -v for version. It seems like the trend is to use -V|--version
> for version, and -v|--verbose for verbosity. I've also seen some tools
> option for `cli version` (subcommand) for version. Anyway, no strong
> preferences from me either.
> 

For the record, bpftool has both "bpftool -V" and "bpftool version" to
dump the version number.

This leaves us with "-v" free to do something like "--verbose", but just
as Jakub said we wanted to limit the risks of confusion... I don't mind
changing, but since nobody has expressed a strong opinion on the matter,
I'll stick to "-d|--debug" for now.

Thanks Andrii for the review!
Quentin

[PATCH bpf-next v2 2/3] libbpf: add bpf_object__load_xattr() API function to pass log_level

[Quentin Monnet]

libbpf was recently made aware of the log_level attribute for programs,
used to specify the level of information expected to be dumped by the
verifier.

Create an API function to pass additional attributes when loading a
bpf_object, so we can set this log_level value in programs when loading
them, and so that so that applications relying on libbpf but not calling
bpf_prog_load_xattr() can also use that feature.

v2:
- We are in a new cycle, bump libbpf extraversion number.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/lib/bpf/Makefile   |  2 +-
 tools/lib/bpf/libbpf.c   | 20 +++++++++++++++++---
 tools/lib/bpf/libbpf.h   |  6 ++++++
 tools/lib/bpf/libbpf.map |  5 +++++
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index a2aceadf68db..9312066a1ae3 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -3,7 +3,7 @@
 
 BPF_VERSION = 0
 BPF_PATCHLEVEL = 0
-BPF_EXTRAVERSION = 3
+BPF_EXTRAVERSION = 4
 
 MAKEFLAGS += --no-print-directory
 
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 197b574406b3..1c6fb7a3201e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2222,7 +2222,7 @@ static bool bpf_program__is_function_storage(struct bpf_program *prog,
 }
 
 static int
-bpf_object__load_progs(struct bpf_object *obj)
+bpf_object__load_progs(struct bpf_object *obj, int log_level)
 {
 	size_t i;
 	int err;
@@ -2230,6 +2230,7 @@ bpf_object__load_progs(struct bpf_object *obj)
 	for (i = 0; i < obj->nr_programs; i++) {
 		if (bpf_program__is_function_storage(&obj->programs[i], obj))
 			continue;
+		obj->programs[i].log_level = log_level;
 		err = bpf_program__load(&obj->programs[i],
 					obj->license,
 					obj->kern_version);
@@ -2381,10 +2382,14 @@ int bpf_object__unload(struct bpf_object *obj)
 	return 0;
 }
 
-int bpf_object__load(struct bpf_object *obj)
+int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 {
+	struct bpf_object *obj;
 	int err;
 
+	if (!attr)
+		return -EINVAL;
+	obj = attr->obj;
 	if (!obj)
 		return -EINVAL;
 
@@ -2397,7 +2402,7 @@ int bpf_object__load(struct bpf_object *obj)
 
 	CHECK_ERR(bpf_object__create_maps(obj), err, out);
 	CHECK_ERR(bpf_object__relocate(obj), err, out);
-	CHECK_ERR(bpf_object__load_progs(obj), err, out);
+	CHECK_ERR(bpf_object__load_progs(obj, attr->log_level), err, out);
 
 	return 0;
 out:
@@ -2406,6 +2411,15 @@ int bpf_object__load(struct bpf_object *obj)
 	return err;
 }
 
+int bpf_object__load(struct bpf_object *obj)
+{
+	struct bpf_object_load_attr attr = {
+		.obj = obj,
+	};
+
+	return bpf_object__load_xattr(&attr);
+}
+
 static int check_path(const char *path)
 {
 	char *cp, errmsg[STRERR_BUFSIZE];
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index c5ff00515ce7..e1c748db44f6 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -89,8 +89,14 @@ LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
 LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path);
 LIBBPF_API void bpf_object__close(struct bpf_object *object);
 
+struct bpf_object_load_attr {
+	struct bpf_object *obj;
+	int log_level;
+};
+
 /* Load/unload object into/from kernel */
 LIBBPF_API int bpf_object__load(struct bpf_object *obj);
+LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr);
 LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
 LIBBPF_API const char *bpf_object__name(struct bpf_object *obj);
 LIBBPF_API unsigned int bpf_object__kversion(struct bpf_object *obj);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 673001787cba..6ce61fa0baf3 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -164,3 +164,8 @@ LIBBPF_0.0.3 {
 		bpf_map_freeze;
 		btf__finalize_data;
 } LIBBPF_0.0.2;
+
+LIBBPF_0.0.4 {
+	global:
+		bpf_object__load_xattr;
+} LIBBPF_0.0.3;
-- 
2.17.1

Re: [PATCH bpf-next v2 2/3] libbpf: add bpf_object__load_xattr() API function to pass log_level

[Yonghong Song]

On 5/23/19 3:54 AM, Quentin Monnet wrote:
> libbpf was recently made aware of the log_level attribute for programs,
> used to specify the level of information expected to be dumped by the
> verifier.
> 
> Create an API function to pass additional attributes when loading a
> bpf_object, so we can set this log_level value in programs when loading
> them, and so that so that applications relying on libbpf but not calling
"so that so that" => "so that"
> bpf_prog_load_xattr() can also use that feature.

Do not fully understand the above statement. From the code below,
I did not see how the non-zero log_level can be set for bpf_program
without bpf_prog_load_xattr(). Maybe I miss something?

> 
> v2:
> - We are in a new cycle, bump libbpf extraversion number.
> 
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>   tools/lib/bpf/Makefile   |  2 +-
>   tools/lib/bpf/libbpf.c   | 20 +++++++++++++++++---
>   tools/lib/bpf/libbpf.h   |  6 ++++++
>   tools/lib/bpf/libbpf.map |  5 +++++
>   4 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index a2aceadf68db..9312066a1ae3 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -3,7 +3,7 @@
>   
>   BPF_VERSION = 0
>   BPF_PATCHLEVEL = 0
> -BPF_EXTRAVERSION = 3
> +BPF_EXTRAVERSION = 4
>   
>   MAKEFLAGS += --no-print-directory
>   
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 197b574406b3..1c6fb7a3201e 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2222,7 +2222,7 @@ static bool bpf_program__is_function_storage(struct bpf_program *prog,
>   }
>   
>   static int
> -bpf_object__load_progs(struct bpf_object *obj)
> +bpf_object__load_progs(struct bpf_object *obj, int log_level)
>   {
>   	size_t i;
>   	int err;
> @@ -2230,6 +2230,7 @@ bpf_object__load_progs(struct bpf_object *obj)
>   	for (i = 0; i < obj->nr_programs; i++) {
>   		if (bpf_program__is_function_storage(&obj->programs[i], obj))
>   			continue;
> +		obj->programs[i].log_level = log_level;
>   		err = bpf_program__load(&obj->programs[i],
>   					obj->license,
>   					obj->kern_version);
> @@ -2381,10 +2382,14 @@ int bpf_object__unload(struct bpf_object *obj)
>   	return 0;
>   }
>   
> -int bpf_object__load(struct bpf_object *obj)
> +int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>   {
> +	struct bpf_object *obj;
>   	int err;
>   
> +	if (!attr)
> +		return -EINVAL;
> +	obj = attr->obj;
>   	if (!obj)
>   		return -EINVAL;
>   
> @@ -2397,7 +2402,7 @@ int bpf_object__load(struct bpf_object *obj)
>   
>   	CHECK_ERR(bpf_object__create_maps(obj), err, out);
>   	CHECK_ERR(bpf_object__relocate(obj), err, out);
> -	CHECK_ERR(bpf_object__load_progs(obj), err, out);
> +	CHECK_ERR(bpf_object__load_progs(obj, attr->log_level), err, out);
>   
>   	return 0;
>   out:
> @@ -2406,6 +2411,15 @@ int bpf_object__load(struct bpf_object *obj)
>   	return err;
>   }
>   
> +int bpf_object__load(struct bpf_object *obj)
> +{
> +	struct bpf_object_load_attr attr = {
> +		.obj = obj,
> +	};
> +
> +	return bpf_object__load_xattr(&attr);
> +}
> +
>   static int check_path(const char *path)
>   {
>   	char *cp, errmsg[STRERR_BUFSIZE];
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index c5ff00515ce7..e1c748db44f6 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -89,8 +89,14 @@ LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
>   LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path);
>   LIBBPF_API void bpf_object__close(struct bpf_object *object);
>   
> +struct bpf_object_load_attr {
> +	struct bpf_object *obj;
> +	int log_level;
> +};
> +
>   /* Load/unload object into/from kernel */
>   LIBBPF_API int bpf_object__load(struct bpf_object *obj);
> +LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr);
>   LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
>   LIBBPF_API const char *bpf_object__name(struct bpf_object *obj);
>   LIBBPF_API unsigned int bpf_object__kversion(struct bpf_object *obj);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 673001787cba..6ce61fa0baf3 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -164,3 +164,8 @@ LIBBPF_0.0.3 {
>   		bpf_map_freeze;
>   		btf__finalize_data;
>   } LIBBPF_0.0.2;
> +
> +LIBBPF_0.0.4 {
> +	global:
> +		bpf_object__load_xattr;
> +} LIBBPF_0.0.3;
> 

Re: [PATCH bpf-next v2 2/3] libbpf: add bpf_object__load_xattr() API function to pass log_level

[Yonghong Song]

On 5/23/19 9:19 AM, Yonghong Song wrote:
> 
> 
> On 5/23/19 3:54 AM, Quentin Monnet wrote:
>> libbpf was recently made aware of the log_level attribute for programs,
>> used to specify the level of information expected to be dumped by the
>> verifier.
>>
>> Create an API function to pass additional attributes when loading a
>> bpf_object, so we can set this log_level value in programs when loading
>> them, and so that so that applications relying on libbpf but not calling
> "so that so that" => "so that"
>> bpf_prog_load_xattr() can also use that feature.
> 
> Do not fully understand the above statement. From the code below,
> I did not see how the non-zero log_level can be set for bpf_program
> without bpf_prog_load_xattr(). Maybe I miss something?

Looks like next patch uses it when -d is specified.
Probably commit message can be made more clear.

> 
>>
>> v2:
>> - We are in a new cycle, bump libbpf extraversion number.
>>
>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> ---
>>   tools/lib/bpf/Makefile   |  2 +-
>>   tools/lib/bpf/libbpf.c   | 20 +++++++++++++++++---
>>   tools/lib/bpf/libbpf.h   |  6 ++++++
>>   tools/lib/bpf/libbpf.map |  5 +++++
>>   4 files changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>> index a2aceadf68db..9312066a1ae3 100644
>> --- a/tools/lib/bpf/Makefile
>> +++ b/tools/lib/bpf/Makefile
>> @@ -3,7 +3,7 @@
>>   BPF_VERSION = 0
>>   BPF_PATCHLEVEL = 0
>> -BPF_EXTRAVERSION = 3
>> +BPF_EXTRAVERSION = 4
>>   MAKEFLAGS += --no-print-directory
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 197b574406b3..1c6fb7a3201e 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -2222,7 +2222,7 @@ static bool 
>> bpf_program__is_function_storage(struct bpf_program *prog,
>>   }
>>   static int
>> -bpf_object__load_progs(struct bpf_object *obj)
>> +bpf_object__load_progs(struct bpf_object *obj, int log_level)
>>   {
>>       size_t i;
>>       int err;
>> @@ -2230,6 +2230,7 @@ bpf_object__load_progs(struct bpf_object *obj)
>>       for (i = 0; i < obj->nr_programs; i++) {
>>           if (bpf_program__is_function_storage(&obj->programs[i], obj))
>>               continue;
>> +        obj->programs[i].log_level = log_level;
>>           err = bpf_program__load(&obj->programs[i],
>>                       obj->license,
>>                       obj->kern_version);
>> @@ -2381,10 +2382,14 @@ int bpf_object__unload(struct bpf_object *obj)
>>       return 0;
>>   }
>> -int bpf_object__load(struct bpf_object *obj)
>> +int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>>   {
>> +    struct bpf_object *obj;
>>       int err;
>> +    if (!attr)
>> +        return -EINVAL;
>> +    obj = attr->obj;
>>       if (!obj)
>>           return -EINVAL;
>> @@ -2397,7 +2402,7 @@ int bpf_object__load(struct bpf_object *obj)
>>       CHECK_ERR(bpf_object__create_maps(obj), err, out);
>>       CHECK_ERR(bpf_object__relocate(obj), err, out);
>> -    CHECK_ERR(bpf_object__load_progs(obj), err, out);
>> +    CHECK_ERR(bpf_object__load_progs(obj, attr->log_level), err, out);
>>       return 0;
>>   out:
>> @@ -2406,6 +2411,15 @@ int bpf_object__load(struct bpf_object *obj)
>>       return err;
>>   }
>> +int bpf_object__load(struct bpf_object *obj)
>> +{
>> +    struct bpf_object_load_attr attr = {
>> +        .obj = obj,
>> +    };
>> +
>> +    return bpf_object__load_xattr(&attr);
>> +}
>> +
>>   static int check_path(const char *path)
>>   {
>>       char *cp, errmsg[STRERR_BUFSIZE];
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index c5ff00515ce7..e1c748db44f6 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -89,8 +89,14 @@ LIBBPF_API int bpf_object__unpin_programs(struct 
>> bpf_object *obj,
>>   LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char 
>> *path);
>>   LIBBPF_API void bpf_object__close(struct bpf_object *object);
>> +struct bpf_object_load_attr {
>> +    struct bpf_object *obj;
>> +    int log_level;
>> +};
>> +
>>   /* Load/unload object into/from kernel */
>>   LIBBPF_API int bpf_object__load(struct bpf_object *obj);
>> +LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr 
>> *attr);
>>   LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
>>   LIBBPF_API const char *bpf_object__name(struct bpf_object *obj);
>>   LIBBPF_API unsigned int bpf_object__kversion(struct bpf_object *obj);
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index 673001787cba..6ce61fa0baf3 100644
>> --- a/tools/lib/bpf/libbpf.map
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -164,3 +164,8 @@ LIBBPF_0.0.3 {
>>           bpf_map_freeze;
>>           btf__finalize_data;
>>   } LIBBPF_0.0.2;
>> +
>> +LIBBPF_0.0.4 {
>> +    global:
>> +        bpf_object__load_xattr;
>> +} LIBBPF_0.0.3;
>>

Re: [PATCH bpf-next v2 2/3] libbpf: add bpf_object__load_xattr() API function to pass log_level

[Quentin Monnet]

2019-05-23 16:29 UTC+0000 ~ Yonghong Song <yhs@fb.com>
> 
> 
> On 5/23/19 9:19 AM, Yonghong Song wrote:
>>
>>
>> On 5/23/19 3:54 AM, Quentin Monnet wrote:
>>> libbpf was recently made aware of the log_level attribute for programs,
>>> used to specify the level of information expected to be dumped by the
>>> verifier.
>>>
>>> Create an API function to pass additional attributes when loading a
>>> bpf_object, so we can set this log_level value in programs when loading
>>> them, and so that so that applications relying on libbpf but not calling
>> "so that so that" => "so that"

Oh, thanks!

>>> bpf_prog_load_xattr() can also use that feature.
>>
>> Do not fully understand the above statement. From the code below,
>> I did not see how the non-zero log_level can be set for bpf_program
>> without bpf_prog_load_xattr(). Maybe I miss something?

bpf_prog_load_xattr() already had support for passing a log_level, it
was added by Alexei when he made libbpf aware of the different log
levels (commit da11b417583e). But bpftool does not rely on
bpf_prog_load_xattr(), it loads programs with bpf_object__load(), that
offered no way pass a log_level parameter. Does that help?

> 
> Looks like next patch uses it when -d is specified.

It uses the new bpf_object__load_xattr() function introduced in this
patch indeed.

> Probably commit message can be made more clear.

Yeah, probably. I'll improve it and submit a v3.

Thanks!
Quentin

[PATCH bpf-next v2 3/3] tools: bpftool: make -d option print debug output from verifier

[Quentin Monnet]

The "-d" option is used to require all logs available for bpftool. So
far it meant telling libbpf to print even debug-level information. But
there is another source of info that can be made more verbose: when we
attemt to load programs with bpftool, we can pass a log_level parameter
to the verifier in order to control the amount of information that is
printed to the console.

Reuse the "-d" option to print all information the verifier can tell. At
this time, this means logs related to BPF_LOG_LEVEL1, BPF_LOG_LEVEL2 and
BPF_LOG_STATS. As mentioned in the discussion on the first version of
this set, these macros are internal to the kernel
(include/linux/bpf_verifier.h) and are not meant to be part of the
stable user API, therefore we simply use the related constants to print
whatever we can at this time, without trying to tell users what is
log_level1 or what is statistics.

Verifier logs are only used when loading programs for now (in the
future: for loading BTF objects with bpftool?), so bpftool.rst and
bpftool-prog.rst are the only man pages to get the update.

v2:
- Remove the possibility to select the log levels to use (v1 offered a
  combination of "log_level1", "log_level2" and "stats").
- The macros from kernel header bpf_verifier.h are not used (and
  therefore not moved to UAPI header).
- In v1 this was a distinct option, but is now merged in the only "-d"
  switch to activate libbpf and verifier debug-level logs all at the
  same time.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../bpftool/Documentation/bpftool-prog.rst    |  5 ++--
 tools/bpf/bpftool/Documentation/bpftool.rst   |  5 ++--
 tools/bpf/bpftool/main.c                      |  2 ++
 tools/bpf/bpftool/main.h                      |  1 +
 tools/bpf/bpftool/prog.c                      | 27 ++++++++++++-------
 5 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 9a92614569e6..228a5c863cc7 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -175,8 +175,9 @@ OPTIONS
 		  (such as tracefs or BPF virtual file system) when necessary.
 
 	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
+		  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
 ========
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index 43dba0717953..6a9c52ef84a9 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -67,8 +67,9 @@ OPTIONS
 		  (such as tracefs or BPF virtual file system) when necessary.
 
 	-d, --debug
-		  Print all logs available from libbpf, including debug-level
-		  information.
+		  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
 ========
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index d74293938a05..4879f6395c7e 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -26,6 +26,7 @@ bool pretty_output;
 bool json_output;
 bool show_pinned;
 bool block_mount;
+bool verifier_logs;
 int bpf_flags;
 struct pinned_obj_table prog_table;
 struct pinned_obj_table map_table;
@@ -373,6 +374,7 @@ int main(int argc, char **argv)
 			break;
 		case 'd':
 			libbpf_set_print(print_all_levels);
+			verifier_logs = true;
 			break;
 		default:
 			p_err("unrecognized option '%s'", argv[optind - 1]);
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 3d63feb7f852..28a2a5857e14 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -91,6 +91,7 @@ extern json_writer_t *json_wtr;
 extern bool json_output;
 extern bool show_pinned;
 extern bool block_mount;
+extern bool verifier_logs;
 extern int bpf_flags;
 extern struct pinned_obj_table prog_table;
 extern struct pinned_obj_table map_table;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 26336bad0442..1f209c80d906 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -750,10 +750,11 @@ static int do_detach(int argc, char **argv)
 
 static int load_with_options(int argc, char **argv, bool first_prog_only)
 {
-	enum bpf_attach_type expected_attach_type;
-	struct bpf_object_open_attr attr = {
-		.prog_type	= BPF_PROG_TYPE_UNSPEC,
+	struct bpf_object_load_attr load_attr = { 0 };
+	struct bpf_object_open_attr open_attr = {
+		.prog_type = BPF_PROG_TYPE_UNSPEC,
 	};
+	enum bpf_attach_type expected_attach_type;
 	struct map_replace *map_replace = NULL;
 	struct bpf_program *prog = NULL, *pos;
 	unsigned int old_map_fds = 0;
@@ -767,7 +768,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 
 	if (!REQ_ARGS(2))
 		return -1;
-	attr.file = GET_ARG();
+	open_attr.file = GET_ARG();
 	pinfile = GET_ARG();
 
 	while (argc) {
@@ -776,7 +777,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 
 			NEXT_ARG();
 
-			if (attr.prog_type != BPF_PROG_TYPE_UNSPEC) {
+			if (open_attr.prog_type != BPF_PROG_TYPE_UNSPEC) {
 				p_err("program type already specified");
 				goto err_free_reuse_maps;
 			}
@@ -793,7 +794,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 			strcat(type, *argv);
 			strcat(type, "/");
 
-			err = libbpf_prog_type_by_name(type, &attr.prog_type,
+			err = libbpf_prog_type_by_name(type,
+						       &open_attr.prog_type,
 						       &expected_attach_type);
 			free(type);
 			if (err < 0)
@@ -881,16 +883,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 
 	set_max_rlimit();
 
-	obj = __bpf_object__open_xattr(&attr, bpf_flags);
+	obj = __bpf_object__open_xattr(&open_attr, bpf_flags);
 	if (IS_ERR_OR_NULL(obj)) {
 		p_err("failed to open object file");
 		goto err_free_reuse_maps;
 	}
 
 	bpf_object__for_each_program(pos, obj) {
-		enum bpf_prog_type prog_type = attr.prog_type;
+		enum bpf_prog_type prog_type = open_attr.prog_type;
 
-		if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
+		if (open_attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
 			const char *sec_name = bpf_program__title(pos, false);
 
 			err = libbpf_prog_type_by_name(sec_name, &prog_type,
@@ -960,7 +962,12 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 		goto err_close_obj;
 	}
 
-	err = bpf_object__load(obj);
+	load_attr.obj = obj;
+	if (verifier_logs)
+		/* log_level1 + log_level2 + stats, but not stable UAPI */
+		load_attr.log_level = 1 + 2 + 4;
+
+	err = bpf_object__load_xattr(&load_attr);
 	if (err) {
 		p_err("failed to load object file");
 		goto err_close_obj;
-- 
2.17.1

Re: [PATCH bpf-next v2 3/3] tools: bpftool: make -d option print debug output from verifier

[Yonghong Song]

On 5/23/19 3:54 AM, Quentin Monnet wrote:
> The "-d" option is used to require all logs available for bpftool. So
> far it meant telling libbpf to print even debug-level information. But
> there is another source of info that can be made more verbose: when we
> attemt to load programs with bpftool, we can pass a log_level parameter
> to the verifier in order to control the amount of information that is
> printed to the console.
> 
> Reuse the "-d" option to print all information the verifier can tell. At
> this time, this means logs related to BPF_LOG_LEVEL1, BPF_LOG_LEVEL2 and
> BPF_LOG_STATS. As mentioned in the discussion on the first version of
> this set, these macros are internal to the kernel
> (include/linux/bpf_verifier.h) and are not meant to be part of the
> stable user API, therefore we simply use the related constants to print
> whatever we can at this time, without trying to tell users what is
> log_level1 or what is statistics.
> 
> Verifier logs are only used when loading programs for now (in the
> future: for loading BTF objects with bpftool?), so bpftool.rst and
> bpftool-prog.rst are the only man pages to get the update.

The current BTF error log print out at warning level. So by default,
it should print out error logs unless it is suppressed explicitly.

int btf__load(struct btf *btf)
{
         __u32 log_buf_size = BPF_LOG_BUF_SIZE;
         char *log_buf = NULL;
         int err = 0;

         if (btf->fd >= 0)
                 return -EEXIST;

         log_buf = malloc(log_buf_size);
         if (!log_buf)
                 return -ENOMEM;

         *log_buf = 0;

         btf->fd = bpf_load_btf(btf->data, btf->data_size,
                                log_buf, log_buf_size, false);
         if (btf->fd < 0) {
                 err = -errno;
                 pr_warning("Error loading BTF: %s(%d)\n", 
strerror(errno), errno);
                 if (*log_buf)
                         pr_warning("%s\n", log_buf);
                 goto done;
         }

done:
         free(log_buf);
         return err;
}

> 
> v2:
> - Remove the possibility to select the log levels to use (v1 offered a
>    combination of "log_level1", "log_level2" and "stats").
> - The macros from kernel header bpf_verifier.h are not used (and
>    therefore not moved to UAPI header).
> - In v1 this was a distinct option, but is now merged in the only "-d"
>    switch to activate libbpf and verifier debug-level logs all at the
>    same time.
> 
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>   .../bpftool/Documentation/bpftool-prog.rst    |  5 ++--
>   tools/bpf/bpftool/Documentation/bpftool.rst   |  5 ++--
>   tools/bpf/bpftool/main.c                      |  2 ++
>   tools/bpf/bpftool/main.h                      |  1 +
>   tools/bpf/bpftool/prog.c                      | 27 ++++++++++++-------
>   5 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index 9a92614569e6..228a5c863cc7 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -175,8 +175,9 @@ OPTIONS
>   		  (such as tracefs or BPF virtual file system) when necessary.
>   
>   	-d, --debug
> -		  Print all logs available from libbpf, including debug-level
> -		  information.
> +		  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
>   ========
> diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
> index 43dba0717953..6a9c52ef84a9 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
> @@ -67,8 +67,9 @@ OPTIONS
>   		  (such as tracefs or BPF virtual file system) when necessary.
>   
>   	-d, --debug
> -		  Print all logs available from libbpf, including debug-level
> -		  information.
> +		  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
>   ========
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index d74293938a05..4879f6395c7e 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -26,6 +26,7 @@ bool pretty_output;
>   bool json_output;
>   bool show_pinned;
>   bool block_mount;
> +bool verifier_logs;
>   int bpf_flags;
>   struct pinned_obj_table prog_table;
>   struct pinned_obj_table map_table;
> @@ -373,6 +374,7 @@ int main(int argc, char **argv)
>   			break;
>   		case 'd':
>   			libbpf_set_print(print_all_levels);
> +			verifier_logs = true;
>   			break;
>   		default:
>   			p_err("unrecognized option '%s'", argv[optind - 1]);
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 3d63feb7f852..28a2a5857e14 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -91,6 +91,7 @@ extern json_writer_t *json_wtr;
>   extern bool json_output;
>   extern bool show_pinned;
>   extern bool block_mount;
> +extern bool verifier_logs;
>   extern int bpf_flags;
>   extern struct pinned_obj_table prog_table;
>   extern struct pinned_obj_table map_table;
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 26336bad0442..1f209c80d906 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -750,10 +750,11 @@ static int do_detach(int argc, char **argv)
>   
>   static int load_with_options(int argc, char **argv, bool first_prog_only)
>   {
> -	enum bpf_attach_type expected_attach_type;
> -	struct bpf_object_open_attr attr = {
> -		.prog_type	= BPF_PROG_TYPE_UNSPEC,
> +	struct bpf_object_load_attr load_attr = { 0 };
> +	struct bpf_object_open_attr open_attr = {
> +		.prog_type = BPF_PROG_TYPE_UNSPEC,
>   	};
> +	enum bpf_attach_type expected_attach_type;
>   	struct map_replace *map_replace = NULL;
>   	struct bpf_program *prog = NULL, *pos;
>   	unsigned int old_map_fds = 0;
> @@ -767,7 +768,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>   
>   	if (!REQ_ARGS(2))
>   		return -1;
> -	attr.file = GET_ARG();
> +	open_attr.file = GET_ARG();
>   	pinfile = GET_ARG();
>   
>   	while (argc) {
> @@ -776,7 +777,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>   
>   			NEXT_ARG();
>   
> -			if (attr.prog_type != BPF_PROG_TYPE_UNSPEC) {
> +			if (open_attr.prog_type != BPF_PROG_TYPE_UNSPEC) {
>   				p_err("program type already specified");
>   				goto err_free_reuse_maps;
>   			}
> @@ -793,7 +794,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>   			strcat(type, *argv);
>   			strcat(type, "/");
>   
> -			err = libbpf_prog_type_by_name(type, &attr.prog_type,
> +			err = libbpf_prog_type_by_name(type,
> +						       &open_attr.prog_type,
>   						       &expected_attach_type);
>   			free(type);
>   			if (err < 0)
> @@ -881,16 +883,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>   
>   	set_max_rlimit();
>   
> -	obj = __bpf_object__open_xattr(&attr, bpf_flags);
> +	obj = __bpf_object__open_xattr(&open_attr, bpf_flags);
>   	if (IS_ERR_OR_NULL(obj)) {
>   		p_err("failed to open object file");
>   		goto err_free_reuse_maps;
>   	}
>   
>   	bpf_object__for_each_program(pos, obj) {
> -		enum bpf_prog_type prog_type = attr.prog_type;
> +		enum bpf_prog_type prog_type = open_attr.prog_type;
>   
> -		if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> +		if (open_attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
>   			const char *sec_name = bpf_program__title(pos, false);
>   
>   			err = libbpf_prog_type_by_name(sec_name, &prog_type,
> @@ -960,7 +962,12 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>   		goto err_close_obj;
>   	}
>   
> -	err = bpf_object__load(obj);
> +	load_attr.obj = obj;
> +	if (verifier_logs)
> +		/* log_level1 + log_level2 + stats, but not stable UAPI */
> +		load_attr.log_level = 1 + 2 + 4;
> +
> +	err = bpf_object__load_xattr(&load_attr);
>   	if (err) {
>   		p_err("failed to load object file");
>   		goto err_close_obj;
> 

Re: [PATCH bpf-next v2 3/3] tools: bpftool: make -d option print debug output from verifier

[Quentin Monnet]

2019-05-23 16:38 UTC+0000 ~ Yonghong Song <yhs@fb.com>
> 
> 
> On 5/23/19 3:54 AM, Quentin Monnet wrote:
>> The "-d" option is used to require all logs available for bpftool. So
>> far it meant telling libbpf to print even debug-level information. But
>> there is another source of info that can be made more verbose: when we
>> attemt to load programs with bpftool, we can pass a log_level parameter
>> to the verifier in order to control the amount of information that is
>> printed to the console.
>>
>> Reuse the "-d" option to print all information the verifier can tell. At
>> this time, this means logs related to BPF_LOG_LEVEL1, BPF_LOG_LEVEL2 and
>> BPF_LOG_STATS. As mentioned in the discussion on the first version of
>> this set, these macros are internal to the kernel
>> (include/linux/bpf_verifier.h) and are not meant to be part of the
>> stable user API, therefore we simply use the related constants to print
>> whatever we can at this time, without trying to tell users what is
>> log_level1 or what is statistics.
>>
>> Verifier logs are only used when loading programs for now (in the
>> future: for loading BTF objects with bpftool?), so bpftool.rst and
>> bpftool-prog.rst are the only man pages to get the update.
> 
> The current BTF error log print out at warning level. So by default,
> it should print out error logs unless it is suppressed explicitly.

Understood, thanks! This is probably something we could change in
libbpf, to have it printed for debug log-level even when no error
occurred. But bpftool does not support loading BTF just yet anyway, so I
suppose it can wait.

Thanks for the review!
Quentin