On Fri, Jul 23, 2021 at 9:13 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote: > > 2021-07-23 08:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > > On Fri, Jul 23, 2021 at 2:31 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote: > >> > >> 2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > >>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> 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().
