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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jul 23, 2021 at 9:13 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 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.

I think you are still confused. It doesn't matter what the user does,
the contract is for libbpf API to either return ERR_PTR(err) if no
CLEAN_PTRS is requested, or return NULL and set errno to -err.
libbpf_err_ptr() does that from inside the libbpf API (so you don't
have to check CLEAN_PTRS explicitly, you are just passing an error to
be returned, regardless of libbpf mode).

If a user opted into CLEAN_PTRS, they don't have to use
libbpf_get_error(), it's enough to check for NULL. If they care about
the error code itself, they'll need to use -errno. If they haven't
opted into CLEAN_PTRS yet, they have to use libbpf_get_error(), as
that's the only supported way. Sure, they could check for NULL and
that's a bug (and that's a very common one, which motivated
CLEAN_PTRS), or they implement the IS_ERR() macro from the kernel
(which is not officially supported, but works, of course). But again,
all that is orthogonal to how libbpf has to return errors from inside
for pointer-returning APIs.

>
> > 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().