Message-ID: <ecc99a33-4049-cd7c-1d53-20c1aa4cced6@gmail.com>
Date:   Thu, 20 Dec 2018 09:17:14 +0900
From:   Taeung Song <treeze.taeung@...il.com>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrey Ignatov <rdna@...com>
Subject: Re: [PATCH bpf-next] libbpf: Show possible section(type) names on
 when failed to guess a type



On 12/19/18 8:26 PM, Quentin Monnet wrote:
> 2018-12-19 17:52 UTC+0900 ~ Taeung Song <treeze.taeung@...il.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/sendmsg6
>>
>> Cc: Jakub Kicinski <jakub.kicinski@...ronome.com>
>> Cc: Andrey Ignatov <rdna@...com>
>> Signed-off-by: Taeung Song <treeze.taeung@...il.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.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);
>>
> 

Powered by blists - more mailing lists