| Message ID | 20190726203747.1124677-5-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Revamp test_progs as a test running framework | expand |
On 07/26, Andrii Nakryiko wrote: > libbpf_swap_print allows to restore previously set print function. > This is useful when running many independent test with one default print > function, but overriding log verbosity for particular subset of tests. Can we change the return type of libbpf_set_print instead and return the old function from it? Will it break ABI? > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > tools/lib/bpf/libbpf.c | 8 ++++++++ > tools/lib/bpf/libbpf.h | 1 + > tools/lib/bpf/libbpf.map | 5 +++++ > 3 files changed, 14 insertions(+) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 8741c39adb1c..0c254b6c9685 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -79,6 +79,14 @@ void libbpf_set_print(libbpf_print_fn_t fn) > __libbpf_pr = fn; > } > > +libbpf_print_fn_t libbpf_swap_print(libbpf_print_fn_t fn) > +{ > + libbpf_print_fn_t old_print_fn = __libbpf_pr; > + > + __libbpf_pr = fn; > + return old_print_fn; > +} > + > __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 5cbf459ece0b..4e0aa893571f 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -58,6 +58,7 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level, > const char *, va_list ap); > > LIBBPF_API void libbpf_set_print(libbpf_print_fn_t fn); > +LIBBPF_API libbpf_print_fn_t libbpf_swap_print(libbpf_print_fn_t fn); > > /* Hide internal to user */ > struct bpf_object; > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index f9d316e873d8..e211c38ddc43 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -184,3 +184,8 @@ LIBBPF_0.0.4 { > perf_buffer__new_raw; > perf_buffer__poll; > } LIBBPF_0.0.3; > + > +LIBBPF_0.0.5 { > + global: > + libbpf_swap_print; > +} LIBBPF_0.0.4; > -- > 2.17.1 >
On Fri, Jul 26, 2019 at 2:28 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 07/26, Andrii Nakryiko wrote: > > libbpf_swap_print allows to restore previously set print function. > > This is useful when running many independent test with one default print > > function, but overriding log verbosity for particular subset of tests. > Can we change the return type of libbpf_set_print instead and return > the old function from it? Will it break ABI? Yeah, thought about that, but I wasn't sure about ABI breakage. It seems like it shouldn't, so I'll just change libbpf_set_print signature instead. > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > tools/lib/bpf/libbpf.c | 8 ++++++++ > > tools/lib/bpf/libbpf.h | 1 + > > tools/lib/bpf/libbpf.map | 5 +++++ > > 3 files changed, 14 insertions(+) > > [...]
On Fri, Jul 26, 2019 at 02:47:28PM -0700, Andrii Nakryiko wrote: > On Fri, Jul 26, 2019 at 2:28 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > On 07/26, Andrii Nakryiko wrote: > > > libbpf_swap_print allows to restore previously set print function. > > > This is useful when running many independent test with one default print > > > function, but overriding log verbosity for particular subset of tests. > > Can we change the return type of libbpf_set_print instead and return > > the old function from it? Will it break ABI? > > Yeah, thought about that, but I wasn't sure about ABI breakage. It > seems like it shouldn't, so I'll just change libbpf_set_print > signature instead. I think it's ok to change return value of libbpf_set_print() from void to useful pointer. This function is not marked as __attribute__((__warn_unused_result__)), so there should be no abi issues. Please double check by compiler perf with different gcc-s as Arnaldo's setup does.
On Fri, Jul 26, 2019 at 5:30 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Jul 26, 2019 at 02:47:28PM -0700, Andrii Nakryiko wrote: > > On Fri, Jul 26, 2019 at 2:28 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > On 07/26, Andrii Nakryiko wrote: > > > > libbpf_swap_print allows to restore previously set print function. > > > > This is useful when running many independent test with one default print > > > > function, but overriding log verbosity for particular subset of tests. > > > Can we change the return type of libbpf_set_print instead and return > > > the old function from it? Will it break ABI? > > > > Yeah, thought about that, but I wasn't sure about ABI breakage. It > > seems like it shouldn't, so I'll just change libbpf_set_print > > signature instead. > > I think it's ok to change return value of libbpf_set_print() from void > to useful pointer. Some googling gave inconclusive results. StackOverflow answers claim it is compatible ABI change ([0]), but I also found some guidelines for Android that designate any return type change as incompatible ([1]). [2] wasn't very helpful about defining compatibility rules, unfortunately. I'm going with [0], though, and changing return type. [0] https://stackoverflow.com/questions/15626579/c-abi-is-changing-a-void-function-to-return-an-int-a-breaking-change [1] https://source.android.com/devices/architecture/vndk/abi-stability [2] https://www.cs.dartmouth.edu/~sergey/cs258/ABI/UlrichDrepper-How-To-Write-Shared-Libraries.pdf > This function is not marked as __attribute__((__warn_unused_result__)), > so there should be no abi issues. > > Please double check by compiler perf with different gcc-s as Arnaldo's setup does. > Compiled (make -C tools/perf) with GCC 4.8.5, GCC 7, and Clang 8. None of them produced any warning, so I'm going forward with just return type change.
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 8741c39adb1c..0c254b6c9685 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -79,6 +79,14 @@ void libbpf_set_print(libbpf_print_fn_t fn) __libbpf_pr = fn; } +libbpf_print_fn_t libbpf_swap_print(libbpf_print_fn_t fn) +{ + libbpf_print_fn_t old_print_fn = __libbpf_pr; + + __libbpf_pr = fn; + return old_print_fn; +} + __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 5cbf459ece0b..4e0aa893571f 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -58,6 +58,7 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level, const char *, va_list ap); LIBBPF_API void libbpf_set_print(libbpf_print_fn_t fn); +LIBBPF_API libbpf_print_fn_t libbpf_swap_print(libbpf_print_fn_t fn); /* Hide internal to user */ struct bpf_object; diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index f9d316e873d8..e211c38ddc43 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -184,3 +184,8 @@ LIBBPF_0.0.4 { perf_buffer__new_raw; perf_buffer__poll; } LIBBPF_0.0.3; + +LIBBPF_0.0.5 { + global: + libbpf_swap_print; +} LIBBPF_0.0.4;
libbpf_swap_print allows to restore previously set print function. This is useful when running many independent test with one default print function, but overriding log verbosity for particular subset of tests. Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/lib/bpf/libbpf.c | 8 ++++++++ tools/lib/bpf/libbpf.h | 1 + tools/lib/bpf/libbpf.map | 5 +++++ 3 files changed, 14 insertions(+)