Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()

[Quentin Monnet <quentin@isovalent.com>]

2021-07-22 17:48 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> Replace the calls to deprecated function btf__get_from_id() with calls
>> to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
>> Update the surrounding code accordingly (instead of passing a pointer to
>> the btf struct, get it as a return value from the function). Also make
>> sure that btf__free() is called on the pointer after use.
>>
>> v2:
>> - Given that btf__load_from_kernel_by_id() has changed since v1, adapt
>>   the code accordingly instead of just renaming the function. Also add a
>>   few calls to btf__free() when necessary.
>>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>> ---
>>  tools/bpf/bpftool/btf.c                      |  8 ++----
>>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
>>  tools/bpf/bpftool/map.c                      | 16 +++++------
>>  tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
>>  tools/perf/util/bpf-event.c                  | 11 ++++----
>>  tools/perf/util/bpf_counter.c                | 12 ++++++--
>>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
>>  7 files changed, 51 insertions(+), 35 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index 09ae0381205b..12787758ce03 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
>>                 }
>>                 return btf_vmlinux;
>>         } else if (info->btf_value_type_id) {
>> -               int err;
>> -
>> -               err = btf__get_from_id(info->btf_id, &btf);
>> -               if (err || !btf) {
>> +               btf = btf__load_from_kernel_by_id(info->btf_id);
>> +               if (libbpf_get_error(btf)) {
>>                         p_err("failed to get btf");
>> -                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
>> +                       if (!btf)
>> +                               btf = ERR_PTR(-ESRCH);
> 
> why not do a simpler (less conditionals)
> 
> err = libbpf_get_error(btf);
> if (err) {
>     btf = ERR_PTR(err);
> }
> 
> ?

Because if btf is NULL at this stage, this would change the return value
from -ESRCH to NULL. This would be problematic in mapdump(), since we
check this value ("if (IS_ERR(btf))") to detect a failure in
get_map_kv_btf().

I could change that check in mapdump() to use libbpf_get_error()
instead, but in that case it would similarly change the return value for
mapdump() (and errno), which I think would be propagated up to main()
and would return 0 instead of -ESRCH. This does not seem suitable and
would play badly with batch mode, among other things.

So I'm considering keeping the one additional if.

> 
>>                 }
>>         }
>>
>> @@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>>                             void *value)
>>  {
>>         json_writer_t *btf_wtr;
>> -       struct btf *btf = NULL;
>> -       int err;
>> +       struct btf *btf;
>>
>> -       err = btf__get_from_id(info->btf_id, &btf);
>> -       if (err) {
>> +       btf = btf__load_from_kernel_by_id(info->btf_id);
>> +       if (libbpf_get_error(btf)) {
>>                 p_err("failed to get btf");
>>                 return;
>>         }
> 
> [...]
> 
>>
>>         func_info = u64_to_ptr(info->func_info);
>> @@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>>                 kernel_syms_destroy(&dd);
>>         }
>>
>> +       btf__free(btf);
>> +
> 
> warrants a Fixes: tag?

I don't mind adding the tags, but do they have any advantage here? My
understanding is that they tend to be neon signs for backports to stable
branches, but this patch depends on btf__load_from_kernel_by_id(),
meaning more patches to pull. I'll see if I can move the btf__free()
fixes to a separate commit, maybe.