* [PATCH bpf-next v2] libbpf: Show supported ELF section names on when failed to guess a prog/attach type
@ 2018-12-21 13:22 Taeung Song
2018-12-21 14:47 ` Quentin Monnet
0 siblings, 1 reply; 3+ messages in thread
From: Taeung Song @ 2018-12-21 13:22 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: netdev, Jakub Kicinski, Quentin Monnet, Andrey Ignatov
We need to let users check their wrong ELF section name
with proper ELF section names when failed to get a prog/attach type from it.
Because users can't realize libbpf guess prog/attach types from
given ELF section names.
For example, when a 'cgroup' section name of a BPF program is used,
Before:
$ bpftool prog load bpf-prog.o /sys/fs/bpf/prog1
Error: failed to guess program type based on ELF section name cgroup
After:
libbpf: failed to guess program type based on ELF section name 'cgroup'
libbpf: supported section(type) names are: socket kprobe/ kretprobe/ classifier action tracepoint/ raw_tracepoint/ xdp perf_event lwt_in lwt_out lwt_xmit lwt_seg6local cgroup_skb/ingress cgroup_skb/egress cgroup/skb cgroup/sock cgroup/post_bind4 cgroup/post_bind6 cgroup/dev sockops sk_skb/stream_parser sk_skb/stream_verdict sk_skb sk_msg lirc_mode2 flow_dissector cgroup/bind4 cgroup/bind6 cgroup/connect4 cgroup/connect6 cgroup/sendmsg4 cgroup/sendmsg6
In addtion, add pr_out printing without the "libbpf: " prefix.
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: Quentin Monnet <quentin.monnet@netronome.com>
Cc: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
tools/bpf/bpftool/prog.c | 10 ++----
tools/lib/bpf/libbpf.c | 31 ++++++++++++++-----
tools/lib/bpf/libbpf.h | 3 +-
tools/lib/bpf/test_libbpf.cpp | 2 +-
tools/perf/util/bpf-loader.c | 4 +--
tools/testing/selftests/bpf/test_btf.c | 2 +-
.../testing/selftests/bpf/test_libbpf_open.c | 4 +--
tools/testing/selftests/bpf/test_progs.c | 4 +--
.../selftests/bpf/test_socket_cookie.c | 4 +--
9 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 2d1bb7d6ff51..0640e9bc0ada 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -930,10 +930,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
err = libbpf_prog_type_by_name(type, &attr.prog_type,
&expected_attach_type);
free(type);
- if (err < 0) {
- p_err("unknown program type '%s'", *argv);
+ if (err < 0)
goto err_free_reuse_maps;
- }
+
NEXT_ARG();
} else if (is_prefix(*argv, "map")) {
void *new_map_replace;
@@ -1028,11 +1027,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
err = libbpf_prog_type_by_name(sec_name, &prog_type,
&expected_attach_type);
- if (err < 0) {
- p_err("failed to guess program type based on section name %s\n",
- sec_name);
+ if (err < 0)
goto err_close_obj;
- }
}
bpf_program__set_ifindex(pos, ifindex);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 169e347c76f6..55cb3f7cb3ee 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -68,24 +68,30 @@ static int __base_pr(const char *format, ...)
static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
static __printf(1, 2) libbpf_print_fn_t __pr_debug;
+static __printf(1, 2) libbpf_print_fn_t __pr_out = __base_pr;
+
+#define pr_fmt(fmt) "libbpf: " fmt
#define __pr(func, fmt, ...) \
do { \
if ((func)) \
- (func)("libbpf: " fmt, ##__VA_ARGS__); \
+ (func)(fmt, ##__VA_ARGS__); \
} while (0)
-#define pr_warning(fmt, ...) __pr(__pr_warning, fmt, ##__VA_ARGS__)
-#define pr_info(fmt, ...) __pr(__pr_info, fmt, ##__VA_ARGS__)
-#define pr_debug(fmt, ...) __pr(__pr_debug, fmt, ##__VA_ARGS__)
+#define pr_warning(fmt, ...) __pr(__pr_warning, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_info(fmt, ...) __pr(__pr_info, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_debug(fmt, ...) __pr(__pr_debug, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_out(fmt, ...) __pr(__pr_out, fmt, ##__VA_ARGS__)
void libbpf_set_print(libbpf_print_fn_t warn,
libbpf_print_fn_t info,
- libbpf_print_fn_t debug)
+ libbpf_print_fn_t debug,
+ libbpf_print_fn_t out)
{
__pr_warning = warn;
__pr_info = info;
__pr_debug = debug;
+ __pr_out = out;
}
#define STRERR_BUFSIZE 128
@@ -2682,6 +2688,12 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,
*expected_attach_type = section_names[i].expected_attach_type;
return 0;
}
+
+ pr_warning("failed to guess program type based on ELF section name '%s'\n", name);
+ pr_info("supported section(type) names are:");
+ for (i = 0; i < ARRAY_SIZE(section_names); i++)
+ pr_out(" %s", section_names[i].sec);
+ pr_out("\n");
return -EINVAL;
}
@@ -2701,6 +2713,13 @@ int libbpf_attach_type_by_name(const char *name,
*attach_type = section_names[i].attach_type;
return 0;
}
+
+ pr_warning("failed to guess attach type based on ELF section name '%s'\n", name);
+ pr_info("attachable section(type) names are:");
+ for (i = 0; i < ARRAY_SIZE(section_names); i++)
+ if (section_names[i].is_attachable)
+ pr_out(" %s", section_names[i].sec);
+ pr_out("\n");
return -EINVAL;
}
@@ -2907,8 +2926,6 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
err = bpf_program__identify_section(prog, &prog_type,
&expected_attach_type);
if (err < 0) {
- pr_warning("failed to guess program type based on section name %s\n",
- prog->section_name);
bpf_object__close(obj);
return -EINVAL;
}
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 5f68d7b75215..930c75012785 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -57,7 +57,8 @@ typedef int (*libbpf_print_fn_t)(const char *, ...)
LIBBPF_API void libbpf_set_print(libbpf_print_fn_t warn,
libbpf_print_fn_t info,
- libbpf_print_fn_t debug);
+ libbpf_print_fn_t debug,
+ libbpf_print_fn_t out);
/* Hide internal to user */
struct bpf_object;
diff --git a/tools/lib/bpf/test_libbpf.cpp b/tools/lib/bpf/test_libbpf.cpp
index abf3fc25c9fa..dcaf10c8b1a5 100644
--- a/tools/lib/bpf/test_libbpf.cpp
+++ b/tools/lib/bpf/test_libbpf.cpp
@@ -8,7 +8,7 @@
int main(int argc, char *argv[])
{
/* libbpf.h */
- libbpf_set_print(NULL, NULL, NULL);
+ libbpf_set_print(NULL, NULL, NULL, NULL);
/* bpf.h */
bpf_prog_get_fd_by_id(0);
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index f9ae1a993806..169dc8a7295e 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -61,7 +61,7 @@ bpf__prepare_load_buffer(void *obj_buf, size_t obj_buf_sz, const char *name)
if (!libbpf_initialized) {
libbpf_set_print(libbpf_warning,
libbpf_info,
- libbpf_debug);
+ libbpf_debug, NULL);
libbpf_initialized = true;
}
@@ -81,7 +81,7 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
if (!libbpf_initialized) {
libbpf_set_print(libbpf_warning,
libbpf_info,
- libbpf_debug);
+ libbpf_debug, NULL);
libbpf_initialized = true;
}
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index 8bcd38010582..3b6b51fde1d1 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -5045,7 +5045,7 @@ int main(int argc, char **argv)
return err;
if (args.always_log)
- libbpf_set_print(__base_pr, __base_pr, __base_pr);
+ libbpf_set_print(__base_pr, __base_pr, __base_pr, NULL);
if (args.raw_test)
err |= test_raw();
diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
index 8fcd1c076add..04bf536e5747 100644
--- a/tools/testing/selftests/bpf/test_libbpf_open.c
+++ b/tools/testing/selftests/bpf/test_libbpf_open.c
@@ -120,7 +120,7 @@ int main(int argc, char **argv)
int longindex = 0;
int opt;
- libbpf_set_print(libbpf_warning, libbpf_info, NULL);
+ libbpf_set_print(libbpf_warning, libbpf_info, NULL, NULL);
/* Parse commands line args */
while ((opt = getopt_long(argc, argv, "hDq",
@@ -128,7 +128,7 @@ int main(int argc, char **argv)
switch (opt) {
case 'D':
libbpf_set_print(libbpf_warning, libbpf_info,
- libbpf_debug);
+ libbpf_debug, NULL);
break;
case 'q': /* Use in scripting mode */
verbose = 0;
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 126fc624290d..969582c1f8c2 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1776,9 +1776,9 @@ static void test_reference_tracking()
/* Expect verifier failure if test name has 'fail' */
if (strstr(title, "fail") != NULL) {
- libbpf_set_print(NULL, NULL, NULL);
+ libbpf_set_print(NULL, NULL, NULL, NULL);
err = !bpf_program__load(prog, "GPL", 0);
- libbpf_set_print(printf, printf, NULL);
+ libbpf_set_print(printf, printf, NULL, NULL);
} else {
err = bpf_program__load(prog, "GPL", 0);
}
diff --git a/tools/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.c
index b6c2c605d8c0..27c1576198d7 100644
--- a/tools/testing/selftests/bpf/test_socket_cookie.c
+++ b/tools/testing/selftests/bpf/test_socket_cookie.c
@@ -158,10 +158,8 @@ static int run_test(int cgfd)
bpf_object__for_each_program(prog, pobj) {
prog_name = bpf_program__title(prog, /*needs_copy*/ false);
- if (libbpf_attach_type_by_name(prog_name, &attach_type)) {
- log_err("Unexpected prog: %s", prog_name);
+ if (libbpf_attach_type_by_name(prog_name, &attach_type))
goto err;
- }
err = bpf_prog_attach(bpf_program__fd(prog), cgfd, attach_type,
BPF_F_ALLOW_OVERRIDE);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Show supported ELF section names on when failed to guess a prog/attach type
2018-12-21 13:22 [PATCH bpf-next v2] libbpf: Show supported ELF section names on when failed to guess a prog/attach type Taeung Song
@ 2018-12-21 14:47 ` Quentin Monnet
2019-01-21 7:18 ` Taeung Song
0 siblings, 1 reply; 3+ messages in thread
From: Quentin Monnet @ 2018-12-21 14:47 UTC (permalink / raw)
To: Taeung Song, Daniel Borkmann, Alexei Starovoitov
Cc: netdev, Jakub Kicinski, Andrey Ignatov
2018-12-21 22:22 UTC+0900 ~ Taeung Song <treeze.taeung@gmail.com>
> We need to let users check their wrong ELF section name
> with proper ELF section names when failed to get a prog/attach type from it.
> Because users can't realize libbpf guess prog/attach types from
> given ELF section names.
> For example, when a 'cgroup' section name of a BPF program is used,
>
> Before:
>
> $ bpftool prog load bpf-prog.o /sys/fs/bpf/prog1
> Error: failed to guess program type based on ELF section name cgroup
>
> After:
>
> libbpf: failed to guess program type based on ELF section name 'cgroup'
> libbpf: supported section(type) names are: socket kprobe/ kretprobe/ classifier action tracepoint/ raw_tracepoint/ xdp perf_event lwt_in lwt_out lwt_xmit lwt_seg6local cgroup_skb/ingress cgroup_skb/egress cgroup/skb cgroup/sock cgroup/post_bind4 cgroup/post_bind6 cgroup/dev sockops sk_skb/stream_parser sk_skb/stream_verdict sk_skb sk_msg lirc_mode2 flow_dissector cgroup/bind4 cgroup/bind6 cgroup/connect4 cgroup/connect6 cgroup/sendmsg4 cgroup/sendmsg6
>
> In addtion, add pr_out printing without the "libbpf: " prefix.
>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Cc: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
> tools/bpf/bpftool/prog.c | 10 ++----
> tools/lib/bpf/libbpf.c | 31 ++++++++++++++-----
> tools/lib/bpf/libbpf.h | 3 +-
> tools/lib/bpf/test_libbpf.cpp | 2 +-
> tools/perf/util/bpf-loader.c | 4 +--
> tools/testing/selftests/bpf/test_btf.c | 2 +-
> .../testing/selftests/bpf/test_libbpf_open.c | 4 +--
> tools/testing/selftests/bpf/test_progs.c | 4 +--
> .../selftests/bpf/test_socket_cookie.c | 4 +--
> 9 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 169e347c76f6..55cb3f7cb3ee 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -68,24 +68,30 @@ static int __base_pr(const char *format, ...)
> static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
> static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
> static __printf(1, 2) libbpf_print_fn_t __pr_debug;
> +static __printf(1, 2) libbpf_print_fn_t __pr_out = __base_pr;
> +
> +#define pr_fmt(fmt) "libbpf: " fmt
>
> #define __pr(func, fmt, ...) \
> do { \
> if ((func)) \
> - (func)("libbpf: " fmt, ##__VA_ARGS__); \
> + (func)(fmt, ##__VA_ARGS__); \
> } while (0)
>
> -#define pr_warning(fmt, ...) __pr(__pr_warning, fmt, ##__VA_ARGS__)
> -#define pr_info(fmt, ...) __pr(__pr_info, fmt, ##__VA_ARGS__)
> -#define pr_debug(fmt, ...) __pr(__pr_debug, fmt, ##__VA_ARGS__)
> +#define pr_warning(fmt, ...) __pr(__pr_warning, pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_info(fmt, ...) __pr(__pr_info, pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_debug(fmt, ...) __pr(__pr_debug, pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_out(fmt, ...) __pr(__pr_out, fmt, ##__VA_ARGS__)
>
> void libbpf_set_print(libbpf_print_fn_t warn,
> libbpf_print_fn_t info,
> - libbpf_print_fn_t debug)
> + libbpf_print_fn_t debug,
> + libbpf_print_fn_t out)
> {
> __pr_warning = warn;
> __pr_info = info;
> __pr_debug = debug;
> + __pr_out = out;
> }
>
> #define STRERR_BUFSIZE 128
> @@ -2682,6 +2688,12 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,
> *expected_attach_type = section_names[i].expected_attach_type;
> return 0;
> }
> +
> + pr_warning("failed to guess program type based on ELF section name '%s'\n", name);
> + pr_info("supported section(type) names are:");
> + for (i = 0; i < ARRAY_SIZE(section_names); i++)
> + pr_out(" %s", section_names[i].sec);
> + pr_out("\n");
> return -EINVAL;
> }
>
> @@ -2701,6 +2713,13 @@ int libbpf_attach_type_by_name(const char *name,
> *attach_type = section_names[i].attach_type;
> return 0;
> }
> +
> + pr_warning("failed to guess attach type based on ELF section name '%s'\n", name);
> + pr_info("attachable section(type) names are:");
> + for (i = 0; i < ARRAY_SIZE(section_names); i++)
> + if (section_names[i].is_attachable)
> + pr_out(" %s", section_names[i].sec);
> + pr_out("\n");
> return -EINVAL;
> }
Thanks for the fixes!
However I don't see an easy way to integrate this pr_out() function to
the list of output facilities in libbpf. I have several concerns here:
- "pr_out()" is not a name that means much by itself, since all
functions are used to print out messages (also the names for "fmt" and
"pr_fmt" are not really explicit either).
- Even if we can find a name that better fits the "no prefix" aspect, we
still have three functions that are distinguished by importance level
(error, warn, info), while this one cannot be combined with them and
just exists for formatting purposes. I mean, as a user, when should I
allow for pr_out() output to be printed? If I want info messages, but
not debug messages, should I print it? Or is that lower importance than
debug?...
- Worse, libbpf_set_print() has been UAPI for several versions now, and
I do not believe we can change it at all. Having an additional function
would be possible, but would look cumbersome maybe.
Why not do something else instead, when trying to attach or load the
programs, for example:
char buf[1024] = {};
/* Forge string buf with all available names */
for (i = 0; i < ARRAY_SIZE(section_names); i++) {
if (strlen(buf) + strlen(" ") +
strlen(section_names[i]) + 1 > sizeof(buf))
break;
strcat(buf, " ");
strcat(buf, sections_names[i]);
}
pr_warning("failed to guess attach type...");
/* Now print all names at once in the same p_info() */
pr_info("attachable section(type) names are: %s\n" buf);
So you wouldn't need to add any additional pr_*() function?
Also for your information, bpf-next tree closed yesterday for the merge
window.
>
> @@ -2907,8 +2926,6 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> err = bpf_program__identify_section(prog, &prog_type,
> &expected_attach_type);
> if (err < 0) {
> - pr_warning("failed to guess program type based on section name %s\n",
> - prog->section_name);
> bpf_object__close(obj);
> return -EINVAL;
> }
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 5f68d7b75215..930c75012785 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -57,7 +57,8 @@ typedef int (*libbpf_print_fn_t)(const char *, ...)
>
> LIBBPF_API void libbpf_set_print(libbpf_print_fn_t warn,
> libbpf_print_fn_t info,
> - libbpf_print_fn_t debug);
> + libbpf_print_fn_t debug,
> + libbpf_print_fn_t out);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Show supported ELF section names on when failed to guess a prog/attach type
2018-12-21 14:47 ` Quentin Monnet
@ 2019-01-21 7:18 ` Taeung Song
0 siblings, 0 replies; 3+ messages in thread
From: Taeung Song @ 2019-01-21 7:18 UTC (permalink / raw)
To: Quentin Monnet, Daniel Borkmann, Alexei Starovoitov
Cc: netdev, Jakub Kicinski, Andrey Ignatov
On 12/21/18 11:47 PM, Quentin Monnet wrote:
> 2018-12-21 22:22 UTC+0900 ~ Taeung Song <treeze.taeung@gmail.com>
>> We need to let users check their wrong ELF section name
>> with proper ELF section names when failed to get a prog/attach type from it.
>> Because users can't realize libbpf guess prog/attach types from
>> given ELF section names.
>> For example, when a 'cgroup' section name of a BPF program is used,
>>
>> Before:
>>
>> $ bpftool prog load bpf-prog.o /sys/fs/bpf/prog1
>> Error: failed to guess program type based on ELF section name cgroup
>>
>> After:
>>
>> libbpf: failed to guess program type based on ELF section name 'cgroup'
>> libbpf: supported section(type) names are: socket kprobe/ kretprobe/ classifier action tracepoint/ raw_tracepoint/ xdp perf_event lwt_in lwt_out lwt_xmit lwt_seg6local cgroup_skb/ingress cgroup_skb/egress cgroup/skb cgroup/sock cgroup/post_bind4 cgroup/post_bind6 cgroup/dev sockops sk_skb/stream_parser sk_skb/stream_verdict sk_skb sk_msg lirc_mode2 flow_dissector cgroup/bind4 cgroup/bind6 cgroup/connect4 cgroup/connect6 cgroup/sendmsg4 cgroup/sendmsg6
>>
>> In addtion, add pr_out printing without the "libbpf: " prefix.
>>
>> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
>> Cc: Quentin Monnet <quentin.monnet@netronome.com>
>> Cc: Andrey Ignatov <rdna@fb.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>> tools/bpf/bpftool/prog.c | 10 ++----
>> tools/lib/bpf/libbpf.c | 31 ++++++++++++++-----
>> tools/lib/bpf/libbpf.h | 3 +-
>> tools/lib/bpf/test_libbpf.cpp | 2 +-
>> tools/perf/util/bpf-loader.c | 4 +--
>> tools/testing/selftests/bpf/test_btf.c | 2 +-
>> .../testing/selftests/bpf/test_libbpf_open.c | 4 +--
>> tools/testing/selftests/bpf/test_progs.c | 4 +--
>> .../selftests/bpf/test_socket_cookie.c | 4 +--
>> 9 files changed, 38 insertions(+), 26 deletions(-)
>>
>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 169e347c76f6..55cb3f7cb3ee 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -68,24 +68,30 @@ static int __base_pr(const char *format, ...)
>> static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
>> static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
>> static __printf(1, 2) libbpf_print_fn_t __pr_debug;
>> +static __printf(1, 2) libbpf_print_fn_t __pr_out = __base_pr;
>> +
>> +#define pr_fmt(fmt) "libbpf: " fmt
>>
>> #define __pr(func, fmt, ...) \
>> do { \
>> if ((func)) \
>> - (func)("libbpf: " fmt, ##__VA_ARGS__); \
>> + (func)(fmt, ##__VA_ARGS__); \
>> } while (0)
>>
>> -#define pr_warning(fmt, ...) __pr(__pr_warning, fmt, ##__VA_ARGS__)
>> -#define pr_info(fmt, ...) __pr(__pr_info, fmt, ##__VA_ARGS__)
>> -#define pr_debug(fmt, ...) __pr(__pr_debug, fmt, ##__VA_ARGS__)
>> +#define pr_warning(fmt, ...) __pr(__pr_warning, pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_info(fmt, ...) __pr(__pr_info, pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_debug(fmt, ...) __pr(__pr_debug, pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_out(fmt, ...) __pr(__pr_out, fmt, ##__VA_ARGS__)
>>
>> void libbpf_set_print(libbpf_print_fn_t warn,
>> libbpf_print_fn_t info,
>> - libbpf_print_fn_t debug)
>> + libbpf_print_fn_t debug,
>> + libbpf_print_fn_t out)
>> {
>> __pr_warning = warn;
>> __pr_info = info;
>> __pr_debug = debug;
>> + __pr_out = out;
>> }
>>
>> #define STRERR_BUFSIZE 128
>> @@ -2682,6 +2688,12 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,
>> *expected_attach_type = section_names[i].expected_attach_type;
>> return 0;
>> }
>> +
>> + pr_warning("failed to guess program type based on ELF section name '%s'\n", name);
>> + pr_info("supported section(type) names are:");
>> + for (i = 0; i < ARRAY_SIZE(section_names); i++)
>> + pr_out(" %s", section_names[i].sec);
>> + pr_out("\n");
>> return -EINVAL;
>> }
>>
>> @@ -2701,6 +2713,13 @@ int libbpf_attach_type_by_name(const char *name,
>> *attach_type = section_names[i].attach_type;
>> return 0;
>> }
>> +
>> + pr_warning("failed to guess attach type based on ELF section name '%s'\n", name);
>> + pr_info("attachable section(type) names are:");
>> + for (i = 0; i < ARRAY_SIZE(section_names); i++)
>> + if (section_names[i].is_attachable)
>> + pr_out(" %s", section_names[i].sec);
>> + pr_out("\n");
>> return -EINVAL;
>> }
>
> Thanks for the fixes!
> However I don't see an easy way to integrate this pr_out() function to
> the list of output facilities in libbpf. I have several concerns here:
>
> - "pr_out()" is not a name that means much by itself, since all
> functions are used to print out messages (also the names for "fmt" and
> "pr_fmt" are not really explicit either).
>
> - Even if we can find a name that better fits the "no prefix" aspect, we
> still have three functions that are distinguished by importance level
> (error, warn, info), while this one cannot be combined with them and
> just exists for formatting purposes. I mean, as a user, when should I
> allow for pr_out() output to be printed? If I want info messages, but
> not debug messages, should I print it? Or is that lower importance than
> debug?...
>
> - Worse, libbpf_set_print() has been UAPI for several versions now, and
> I do not believe we can change it at all. Having an additional function
> would be possible, but would look cumbersome maybe.
>
> Why not do something else instead, when trying to attach or load the
> programs, for example:
>
> char buf[1024] = {};
>
> /* Forge string buf with all available names */
> for (i = 0; i < ARRAY_SIZE(section_names); i++) {
> if (strlen(buf) + strlen(" ") +
> strlen(section_names[i]) + 1 > sizeof(buf))
> break;
> strcat(buf, " ");
> strcat(buf, sections_names[i]);
> }
>
> pr_warning("failed to guess attach type...");
> /* Now print all names at once in the same p_info() */
> pr_info("attachable section(type) names are: %s\n" buf);
>
> So you wouldn't need to add any additional pr_*() function?
>
> Also for your information, bpf-next tree closed yesterday for the merge
> window.
>
Thanks for detailed review.
I changed this patch as you said. At last, I send v3 soon.
Thanks,
Taeung
>>
>> @@ -2907,8 +2926,6 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
>> err = bpf_program__identify_section(prog, &prog_type,
>> &expected_attach_type);
>> if (err < 0) {
>> - pr_warning("failed to guess program type based on section name %s\n",
>> - prog->section_name);
>> bpf_object__close(obj);
>> return -EINVAL;
>> }
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 5f68d7b75215..930c75012785 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -57,7 +57,8 @@ typedef int (*libbpf_print_fn_t)(const char *, ...)
>>
>> LIBBPF_API void libbpf_set_print(libbpf_print_fn_t warn,
>> libbpf_print_fn_t info,
>> - libbpf_print_fn_t debug);
>> + libbpf_print_fn_t debug,
>> + libbpf_print_fn_t out);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-01-21 7:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-21 13:22 [PATCH bpf-next v2] libbpf: Show supported ELF section names on when failed to guess a prog/attach type Taeung Song
2018-12-21 14:47 ` Quentin Monnet
2019-01-21 7:18 ` Taeung Song
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).