On 12/19/18 8:26 PM, Quentin Monnet wrote:
2018-12-19 17:52 UTC+0900 ~ Taeung Song <treeze.tae...@gmail.com>We need to let users check their wrong section name with proper section names when failed to get proper type from it. Because there is no knowing what kind of section name can be used. 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 section name cgroup After: libbpf: failed to guess program type based on section name 'cgroup'libbpf: possible 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/sendmsg6Cc: Jakub Kicinski <jakub.kicin...@netronome.com> Cc: Andrey Ignatov <r...@fb.com> Signed-off-by: Taeung Song <treeze.tae...@gmail.com> --- tools/bpf/bpftool/prog.c | 10 +++------- tools/lib/bpf/libbpf.c | 18 ++++++++++++++++-- .../testing/selftests/bpf/test_socket_cookie.c | 4 +--- 3 files changed, 20 insertions(+), 12 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..ad274ac8d630 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -78,6 +78,7 @@ do { \ #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_out(fmt, ...) __base_pr(fmt, ##__VA_ARGS__)Hi, thanks! Would be nice to get existing prog/attach types indeed.Please can we avoid using a macro that unconditionally prints to stderr? What will happen in your code below if any application uses libbpf_set_print() to redefine pr_warning() and pr_info() is that only the existing section names will be printed out, with no accompanying message. Did you add pr_out() to avoid having the "libbpf: " prefix printed in the middle of the list? If so, could we find something else (e.g. alternative __pr() macro) and a way to replace that function by a user-defined one (like libbpf_set_print() offers for the other ones)?My main concern is to avoid breaking JSON output too much for bpftool.
Hi, I got it. I also think it should be changed to unconditionally print warnings.
Alternatively, would that make sense to have instead a function that return a list of supported section names, and that could be called from bpftool as a complement to the current error message? Just an idea.
Yep, I also think the function for a list of supported section names can be used for the 'TYPE' help message (bpftool-prog) Jakub said.
void libbpf_set_print(libbpf_print_fn_t warn, libbpf_print_fn_t info,@@ -2682,6 +2683,13 @@ 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 section name '%s'\n", name);+ pr_info("possible section(type) names are:"); + for (i = 0; i < ARRAY_SIZE(section_names); i++) { + pr_out(" %s", section_names[i].sec); + }Nit: unnecessary brackets.+ pr_out("\n"); return -EINVAL; } @@ -2701,6 +2709,14 @@ 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 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); + }Likewise.+ pr_out("\n"); return -EINVAL; }@@ -2907,8 +2923,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/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.cindex 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);
