[bpf-next] libbpf: configure log verbosity with env variable

Commit Message

Mykyta Yatsenko [e-mail] May 23, 2024, 8:53 p.m. UTC

From: Mykyta Yatsenko <[e-mail]>

Configure logging verbosity by setting LIBBPF_LOG environment
variable. Only applying to default logger. Once user set their custom
logging callback, it is up to them to filter.

Signed-off-by: Mykyta Yatsenko <[e-mail]>
---
 tools/lib/bpf/libbpf.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Andrii Nakryiko May 23, 2024, 9:50 p.m. UTC | #1

On Thu, May 23, 2024 at 1:53 PM Mykyta Yatsenko
<[e-mail]> wrote:
>
> From: Mykyta Yatsenko <[e-mail]>
>
> Configure logging verbosity by setting LIBBPF_LOG environment
> variable. Only applying to default logger. Once user set their custom
> logging callback, it is up to them to filter.
>
> Signed-off-by: Mykyta Yatsenko <[e-mail]>
> ---
>  tools/lib/bpf/libbpf.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 5401f2df463d..8805607073da 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -229,7 +229,23 @@ static const char * const prog_type_name[] = {
>  static int __base_pr(enum libbpf_print_level level, const char *format,
>                      va_list args)
>  {
> -       if (level == LIBBPF_DEBUG)
> +       static enum libbpf_print_level env_level = LIBBPF_INFO;

this doesn't have to come from just the environment, we might add an
API to set the default log level programmatically (in the future, not
saying we should do it right now). Maybe more generic "min_level" or
"log_level"?

> +       static bool initialized;
> +
> +       if (!initialized) {
> +               char *verbosity;
> +
> +               initialized = true;
> +               verbosity = getenv("LIBBPF_LOG");

I'd be confused whether LIBBPF_LOG is log *level* or some sort of file
path to which default log should go (and who knows, maybe we'll add
that). So let's maybe call it LIBBPF_LOG_LEVEL=... ?

> +               if (verbosity) {
> +                       if (strcmp(verbosity, "warn") == 0)

let's do case-insensitive comparison?

> +                               env_level = LIBBPF_WARN;
> +                       else if (strcmp(verbosity, "debug") == 0)
> +                               env_level = LIBBPF_DEBUG;

let's handle the "info" level as well?

and for unrecognized value, let's print a warning directly with fprintf(stderr)?

Let's also add this LIBBPF_LOG_LEVEL envvar description to the doc
comment for libbpf_set_print() and a short section somewhere in
Documentation/bpf/libbpf/libbpf_overview.rst ?

> +               }
> +       }
> +
> +       if (env_level < level)

super nitpicky, sorry, but my brain refuses to intuitively understand
this, can you invert the condition please (and maybe add a comment):

/* if too verbose, skip logging */
if (level > min_level)
    return 0;

But in general this is a useful feature, thanks!

pw-bot: cr

>                 return 0;
>
>         return vfprintf(stderr, format, args);
> --
> 2.45.0
>

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 5401f2df463d..8805607073da 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -229,7 +229,23 @@  static const char * const prog_type_name[] = {
 static int __base_pr(enum libbpf_print_level level, const char *format,
 		     va_list args)
 {
-	if (level == LIBBPF_DEBUG)
+	static enum libbpf_print_level env_level = LIBBPF_INFO;
+	static bool initialized;
+
+	if (!initialized) {
+		char *verbosity;
+
+		initialized = true;
+		verbosity = getenv("LIBBPF_LOG");
+		if (verbosity) {
+			if (strcmp(verbosity, "warn") == 0)
+				env_level = LIBBPF_WARN;
+			else if (strcmp(verbosity, "debug") == 0)
+				env_level = LIBBPF_DEBUG;
+		}
+	}
+
+	if (env_level < level)
 		return 0;
 
 	return vfprintf(stderr, format, args);