* [PATCH bpf-next] libbpf: Add libbpf_set_log_level() function to adjust logging @ 2019-10-24 13:21 Toke Høiland-Jørgensen 2019-10-25 16:58 ` Andrii Nakryiko 0 siblings, 1 reply; 5+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-24 13:21 UTC (permalink / raw) To: daniel, ast; +Cc: Toke Høiland-Jørgensen, bpf, netdev Currently, the only way to change the logging output of libbpf is to override the print function with libbpf_set_print(). This is somewhat cumbersome if one just wants to change the logging level (e.g., to enable debugging), so add another function that just adjusts the default output printing by adjusting the filtering of messages. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- tools/lib/bpf/libbpf.c | 12 +++++++++++- tools/lib/bpf/libbpf.h | 2 ++ tools/lib/bpf/libbpf.map | 1 + 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index d1c4440a678e..93909d9a423d 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -67,10 +67,12 @@ #define __printf(a, b) __attribute__((format(printf, a, b))) +static enum libbpf_print_level __libbpf_log_level = LIBBPF_INFO; + static int __base_pr(enum libbpf_print_level level, const char *format, va_list args) { - if (level == LIBBPF_DEBUG) + if (level > __libbpf_log_level) return 0; return vfprintf(stderr, format, args); @@ -86,6 +88,14 @@ libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn) return old_print_fn; } +enum libbpf_print_level libbpf_set_log_level(enum libbpf_print_level level) +{ + enum libbpf_print_level old_level = __libbpf_log_level; + + __libbpf_log_level = level; + return old_level; +} + __printf(2, 3) void libbpf_print(enum libbpf_print_level level, const char *format, ...) { diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index c63e2ff84abc..0bba6c2259f1 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -58,6 +58,8 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level, const char *, va_list ap); LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn); +LIBBPF_API enum libbpf_print_level +libbpf_set_log_level(enum libbpf_print_level level); /* Hide internal to user */ struct bpf_object; diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index d1473ea4d7a5..c3f79418c2be 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -197,4 +197,5 @@ LIBBPF_0.0.6 { bpf_object__open_mem; bpf_program__get_expected_attach_type; bpf_program__get_type; + libbpf_set_log_level; } LIBBPF_0.0.5; -- 2.23.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: Add libbpf_set_log_level() function to adjust logging 2019-10-24 13:21 [PATCH bpf-next] libbpf: Add libbpf_set_log_level() function to adjust logging Toke Høiland-Jørgensen @ 2019-10-25 16:58 ` Andrii Nakryiko 2019-10-27 11:08 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 5+ messages in thread From: Andrii Nakryiko @ 2019-10-25 16:58 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Networking On Fri, Oct 25, 2019 at 4:50 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Currently, the only way to change the logging output of libbpf is to > override the print function with libbpf_set_print(). This is somewhat > cumbersome if one just wants to change the logging level (e.g., to enable No, it's not. Having one way of doing things is good, proliferation of APIs is not a good thing. Either way you require application to write some additional code. Doing simple vprintf-based (or whatever application is using to print logs, which libbpf shouldn't care about!) function with single if is not hard and is not cumbersome. If you care about helping users to be less confused on how to do that, I think it would be a good idea to have some sort of libbpf-specific FAQ with code samples on how to achieve typical and common stuff, like this one. So please instead consider doing that. > debugging), so add another function that just adjusts the default output > printing by adjusting the filtering of messages. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > tools/lib/bpf/libbpf.c | 12 +++++++++++- > tools/lib/bpf/libbpf.h | 2 ++ > tools/lib/bpf/libbpf.map | 1 + > 3 files changed, 14 insertions(+), 1 deletion(-) > [...] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: Add libbpf_set_log_level() function to adjust logging 2019-10-25 16:58 ` Andrii Nakryiko @ 2019-10-27 11:08 ` Toke Høiland-Jørgensen 2019-10-27 20:00 ` Andrii Nakryiko 0 siblings, 1 reply; 5+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-27 11:08 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Networking Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Fri, Oct 25, 2019 at 4:50 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Currently, the only way to change the logging output of libbpf is to >> override the print function with libbpf_set_print(). This is somewhat >> cumbersome if one just wants to change the logging level (e.g., to enable > > No, it's not. Yes, it is :) > Having one way of doing things is good, proliferation of APIs is not a > good thing. Either way you require application to write some > additional code. Doing simple vprintf-based (or whatever application > is using to print logs, which libbpf shouldn't care about!) function > with single if is not hard and is not cumbersome. The print function registration is fine for applications that want to control its own logging in detail. This patch is about lowering barriers to entry for people who are starting out with libbpf, and just want to find out why their program isn't doing what it's supposed to. Which is not the point to go figure out an arcane function pointer-based debugging setup API just to get some help. Helping users in this situation is the friendly thing to do, and worth the (quite limited) cost of adding this mechanism. If you're objecting to the new API function, an alternative could be to react to an environment variable? I.e., turn on debugging of LIBBPF_DEBUG=1 is in the environment? That way, users wouldn't even have to add the extra function call, they could just re-run their application with the env var set on the command line... > If you care about helping users to be less confused on how to do that, > I think it would be a good idea to have some sort of libbpf-specific > FAQ with code samples on how to achieve typical and common stuff, like > this one. So please instead consider doing that. The fact that you're suggesting putting in a FAQ entry on *how to enable debug logging* should be proof enough that the current API is confusing... -Toke ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: Add libbpf_set_log_level() function to adjust logging 2019-10-27 11:08 ` Toke Høiland-Jørgensen @ 2019-10-27 20:00 ` Andrii Nakryiko 2019-10-27 20:55 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 5+ messages in thread From: Andrii Nakryiko @ 2019-10-27 20:00 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Networking On Sun, Oct 27, 2019 at 4:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Fri, Oct 25, 2019 at 4:50 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> Currently, the only way to change the logging output of libbpf is to > >> override the print function with libbpf_set_print(). This is somewhat > >> cumbersome if one just wants to change the logging level (e.g., to enable > > > > No, it's not. > > Yes, it is :) As much fun as it is to keep exchanging subjective statements, I won't do that. I'll just say that having written a bunch of applications utilizing libbpf and speaking with people (not libbpf contributors) who have write their own apps w/ libbpf, setting up custom logging hook was never mentioned as a problem, not even a single time. I'll go even further and will say that the fact that libbpf (a library) can print out something to application's stderr is already pretty bad behavior and for big production applications is a big no-no. Library shouldn't pollute program's stdout/stderr with extra unsolicited output. We might actually consider changing that eventually. > > > Having one way of doing things is good, proliferation of APIs is not a > > good thing. Either way you require application to write some > > additional code. Doing simple vprintf-based (or whatever application > > is using to print logs, which libbpf shouldn't care about!) function > > with single if is not hard and is not cumbersome. > > The print function registration is fine for applications that want to > control its own logging in detail. > > This patch is about lowering barriers to entry for people who are > starting out with libbpf, and just want to find out why their program > isn't doing what it's supposed to. Which is not the point to go figure > out an arcane function pointer-based debugging setup API just to get Which aspect you find arcane? that's it's a callback? that's it as function pointer? that it's using va_list? what exactly is confusing here? > some help. Helping users in this situation is the friendly thing to do, > and worth the (quite limited) cost of adding this mechanism. It is not a small cost. It's a new mechanism and a new set of conventions, which are not orthogonal to existing logging mechanism and can easily break. If application sets its own debugging callback (and your specific user might not even know about this, because he's working on an application, which has a separate libbpf initialization part done by someone else), this new API will do absolutely nothing, confusing people as much or even more. Further, this is introducing a new global state that we need to maintain. We've been ignoring multi-threading concerns so far, but this will have to stop and we'll need to deal with the need to synchronize things, at least for global state. So adding more global stuff is bad and has its costs. > > If you're objecting to the new API function, an alternative could be to > react to an environment variable? I.e., turn on debugging of > LIBBPF_DEBUG=1 is in the environment? That way, users wouldn't even have > to add the extra function call, they could just re-run their application > with the env var set on the command line... Should this envvar be re-read every time libbpf might log something? Or just once before libbpf is initialized? If the latter, when precisely this envvar should be read? before first bpf_object is open? or we should re-read every time new bpf_object is open? And so on... Or, why not, say, a special agreed-upon (or maybe it should be overridable through extra options) file somewhere, that will control logging verbosity? Or maybe we should support a custom (and, of course, optional) signal handler to be installed so that we can trigger more verbose libbpf output without having to restart a long-running application? All this would be very helpful for some specific subset of situations, but that doesn't mean that libbpf has to support all these custom cases. It already provides a generic and easy to use mechanism to let application decide for itself what it wants to do. And we should keep it that way. > > > If you care about helping users to be less confused on how to do that, > > I think it would be a good idea to have some sort of libbpf-specific > > FAQ with code samples on how to achieve typical and common stuff, like > > this one. So please instead consider doing that. > > The fact that you're suggesting putting in a FAQ entry on *how to enable > debug logging* should be proof enough that the current API is > confusing... No, I'm suggesting, if you really think libbpf's logging is confusing, to rather spend your efforts on writing a tiny piece of documentation explaining how libbpf logging is done and, as a simple example, show how to do verbose logging to stderr. That way you'll eliminate any confusion explicitly, instead of adding another API call that: 1) confused user will still have to find and 2) will now have to figure out why the hell libbpf has two different ways to do logging, one of them working only under a specific set of circumstances. > > -Toke > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: Add libbpf_set_log_level() function to adjust logging 2019-10-27 20:00 ` Andrii Nakryiko @ 2019-10-27 20:55 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 5+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-27 20:55 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Networking Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Sun, Oct 27, 2019 at 4:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Fri, Oct 25, 2019 at 4:50 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> Currently, the only way to change the logging output of libbpf is to >> >> override the print function with libbpf_set_print(). This is somewhat >> >> cumbersome if one just wants to change the logging level (e.g., to enable >> > >> > No, it's not. >> >> Yes, it is :) > > As much fun as it is to keep exchanging subjective statements, I won't > do that. Heh, yeah. Even though I think the current behaviour is incredibly annoying, it's also somewhat of a bikeshedding issue, so let's just agree to disagree on this, drop this patch and move on :) -Toke ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-27 20:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-24 13:21 [PATCH bpf-next] libbpf: Add libbpf_set_log_level() function to adjust logging Toke Høiland-Jørgensen 2019-10-25 16:58 ` Andrii Nakryiko 2019-10-27 11:08 ` Toke Høiland-Jørgensen 2019-10-27 20:00 ` Andrii Nakryiko 2019-10-27 20:55 ` Toke Høiland-Jørgensen
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).