Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()

[Quentin Monnet <quentin@isovalent.com>]

2021-07-23 08:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, Jul 23, 2021 at 2:31 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> 2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>
>>>> Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
>>>> better indicate what the function does. Change the new function so that,
>>>> instead of requiring a pointer to the pointer to update and returning
>>>> with an error code, it takes a single argument (the id of the BTF
>>>> object) and returns the corresponding pointer. This is more in line with
>>>> the existing constructors.
>>>>
>>>> The other tools calling the deprecated btf__get_from_id() function will
>>>> be updated in a future commit.
>>>>
>>>> References:
>>>>
>>>> - https://github.com/libbpf/libbpf/issues/278
>>>> - https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
>>>>

>>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>>> index 7e0de560490e..6654bdee7ad7 100644
>>>> --- a/tools/lib/bpf/btf.c
>>>> +++ b/tools/lib/bpf/btf.c
>>>> @@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
>>>>         return btf;
>>>>  }
>>>>
>>>> +struct btf *btf__load_from_kernel_by_id(__u32 id)
>>>> +{
>>>> +       struct btf *btf;
>>>> +       int btf_fd;
>>>> +
>>>> +       btf_fd = bpf_btf_get_fd_by_id(id);
>>>> +       if (btf_fd < 0)
>>>> +               return ERR_PTR(-errno);
>>>
>>> please use libbpf_err_ptr() for consistency, see
>>> bpf_object__open_mem() for an example
>>
>> I can do that, but I'll need to uncouple btf__get_from_id() from the new
>> function. If it calls btf__load_from_kernel_by_id() and
>> LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.
> 
> No it won't, if libbpf_get_error() is used right after the API call.

But we cannot be sure that users currently call libbpf_get_error() after
btf__get_from_id()? I'm fine if we assume they do (users currently
selecting the CLEAN_PTRS are probably savvy enough to call it I guess),
I'll update as you suggest.

> With CLEAN_PTRS the result pointer is NULL but actual error is passed
> through errno. libbpf_get_error() knows about this and extracts error
> from errno if passed NULL pointer. With returning ERR_PTR(-errno) from
> btf__load_from_kernel_by_id() you are breaking CLEAN_PTRS guarantees.
OK right, this makes sense to me for btf__load_from_kernel_by_id().