| Message ID | 20210721153808.6902-3-quentin@isovalent.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 4 maintainers not CCed: songliubraving@fb.com yhs@fb.com kafai@fb.com kpsingh@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 70 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 > > v2: > - Instead of a simple renaming, change the new function to make it > return the pointer to the btf struct. > - API v0.5.0 instead of v0.6.0. We generally keep such version changes to cover letters. It keeps each individual commit clean and collects full history in the cover letter which becomes a body of merge commit when the whole patch set is applied. For next revision please consolidate the history in the cover letter. Thanks! > > Signed-off-by: Quentin Monnet <quentin@isovalent.com> > Acked-by: John Fastabend <john.fastabend@gmail.com> > --- > tools/lib/bpf/btf.c | 25 +++++++++++++++++-------- > tools/lib/bpf/btf.h | 1 + > tools/lib/bpf/libbpf.c | 5 +++-- > tools/lib/bpf/libbpf.map | 1 + > 4 files changed, 22 insertions(+), 10 deletions(-) > > 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 > + > + btf = btf_get_from_fd(btf_fd, NULL); > + close(btf_fd); > + > + return libbpf_ptr(btf); > +} > + > int btf__get_from_id(__u32 id, struct btf **btf) > { > struct btf *res; > - int err, btf_fd; > + int err; > > *btf = NULL; > - btf_fd = bpf_btf_get_fd_by_id(id); > - if (btf_fd < 0) > - return libbpf_err(-errno); > - > - res = btf_get_from_fd(btf_fd, NULL); > + res = btf__load_from_kernel_by_id(id); > err = libbpf_get_error(res); > > - close(btf_fd); > - > if (err) > return libbpf_err(err); > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > index fd8a21d936ef..3db9446bc133 100644 > --- a/tools/lib/bpf/btf.h > +++ b/tools/lib/bpf/btf.h > @@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size); > LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset); > LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset); > LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); > +LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id); let's move this definition to after btf__parse() to keep all "constructors" together (we can move btf__get_from_id() there for completeness as well, I suppose). > LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, > __u32 expected_key_size, > __u32 expected_value_size, > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 242e97892043..eff005b1eba1 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) > { > struct bpf_prog_info_linear *info_linear; > struct bpf_prog_info *info; > - struct btf *btf = NULL; > int err = -EINVAL; > + struct btf *btf; > > info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0); > err = libbpf_get_error(info_linear); > @@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) > pr_warn("The target program doesn't have BTF\n"); > goto out; > } > - if (btf__get_from_id(info->btf_id, &btf)) { > + btf = btf__load_from_kernel_by_id(info->btf_id); > + if (libbpf_get_error(btf)) { there seems to be a bug in existing code and you are keeping it. On error err will be 0. Let's fix it. Same for above if (!info->btf_id), please fix that as well while you are at it. > pr_warn("Failed to get BTF of the program\n"); > goto out; > } > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index f7d52d76ca3a..ca8cc7a7faad 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -373,6 +373,7 @@ LIBBPF_0.5.0 { > bpf_map__initial_value; > bpf_map_lookup_and_delete_elem_flags; > bpf_object__gen_loader; > + btf__load_from_kernel_by_id; > btf__load_into_kernel; > btf_dump__dump_type_data; > libbpf_set_strict_mode; > -- > 2.30.2 >
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 >> >> v2: >> - Instead of a simple renaming, change the new function to make it >> return the pointer to the btf struct. >> - API v0.5.0 instead of v0.6.0. > > We generally keep such version changes to cover letters. It keeps each > individual commit clean and collects full history in the cover letter > which becomes a body of merge commit when the whole patch set is > applied. For next revision please consolidate the history in the cover > letter. Thanks! OK will do. I've seen other folks detailing the changes on individual patches, and done so in the past, although it's true the current trend is to have it in the cover letter (and I understand the motivation). > >> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com> >> Acked-by: John Fastabend <john.fastabend@gmail.com> >> --- >> tools/lib/bpf/btf.c | 25 +++++++++++++++++-------- >> tools/lib/bpf/btf.h | 1 + >> tools/lib/bpf/libbpf.c | 5 +++-- >> tools/lib/bpf/libbpf.map | 1 + >> 4 files changed, 22 insertions(+), 10 deletions(-) >> >> 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. > >> + >> + btf = btf_get_from_fd(btf_fd, NULL); >> + close(btf_fd); >> + >> + return libbpf_ptr(btf); >> +} >> + >> int btf__get_from_id(__u32 id, struct btf **btf) >> { >> struct btf *res; >> - int err, btf_fd; >> + int err; >> >> *btf = NULL; >> - btf_fd = bpf_btf_get_fd_by_id(id); >> - if (btf_fd < 0) >> - return libbpf_err(-errno); >> - >> - res = btf_get_from_fd(btf_fd, NULL); >> + res = btf__load_from_kernel_by_id(id); >> err = libbpf_get_error(res); >> >> - close(btf_fd); >> - >> if (err) >> return libbpf_err(err); >> >> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h >> index fd8a21d936ef..3db9446bc133 100644 >> --- a/tools/lib/bpf/btf.h >> +++ b/tools/lib/bpf/btf.h >> @@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size); >> LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset); >> LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset); >> LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); >> +LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id); > > let's move this definition to after btf__parse() to keep all > "constructors" together (we can move btf__get_from_id() there for > completeness as well, I suppose). I thought about that but wasn't sure, OK will do. > >> LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, >> __u32 expected_key_size, >> __u32 expected_value_size, >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 242e97892043..eff005b1eba1 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) >> { >> struct bpf_prog_info_linear *info_linear; >> struct bpf_prog_info *info; >> - struct btf *btf = NULL; >> int err = -EINVAL; >> + struct btf *btf; >> >> info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0); >> err = libbpf_get_error(info_linear); >> @@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) >> pr_warn("The target program doesn't have BTF\n"); >> goto out; >> } >> - if (btf__get_from_id(info->btf_id, &btf)) { >> + btf = btf__load_from_kernel_by_id(info->btf_id); >> + if (libbpf_get_error(btf)) { > > there seems to be a bug in existing code and you are keeping it. On > error err will be 0. Let's fix it. Same for above if (!info->btf_id), > please fix that as well while you are at it. Oh right, I saw that err was initialised at -EINVAL and did not notice it was changed for the info_linear. I'll address it.
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 > >> > >> v2: > >> - Instead of a simple renaming, change the new function to make it > >> return the pointer to the btf struct. > >> - API v0.5.0 instead of v0.6.0. > > > > We generally keep such version changes to cover letters. It keeps each > > individual commit clean and collects full history in the cover letter > > which becomes a body of merge commit when the whole patch set is > > applied. For next revision please consolidate the history in the cover > > letter. Thanks! > > OK will do. > I've seen other folks detailing the changes on individual patches, and > done so in the past, although it's true the current trend is to have it > in the cover letter (and I understand the motivation). > > > > >> > >> Signed-off-by: Quentin Monnet <quentin@isovalent.com> > >> Acked-by: John Fastabend <john.fastabend@gmail.com> > >> --- > >> tools/lib/bpf/btf.c | 25 +++++++++++++++++-------- > >> tools/lib/bpf/btf.h | 1 + > >> tools/lib/bpf/libbpf.c | 5 +++-- > >> tools/lib/bpf/libbpf.map | 1 + > >> 4 files changed, 22 insertions(+), 10 deletions(-) > >> > >> 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. 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. > > > > >> + > >> + btf = btf_get_from_fd(btf_fd, NULL); > >> + close(btf_fd); > >> + > >> + return libbpf_ptr(btf); > >> +} > >> + > >> int btf__get_from_id(__u32 id, struct btf **btf) > >> { > >> struct btf *res; > >> - int err, btf_fd; > >> + int err; > >> > >> *btf = NULL; > >> - btf_fd = bpf_btf_get_fd_by_id(id); > >> - if (btf_fd < 0) > >> - return libbpf_err(-errno); > >> - > >> - res = btf_get_from_fd(btf_fd, NULL); > >> + res = btf__load_from_kernel_by_id(id); > >> err = libbpf_get_error(res); > >> > >> - close(btf_fd); > >> - > >> if (err) > >> return libbpf_err(err); > >> > >> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > >> index fd8a21d936ef..3db9446bc133 100644 > >> --- a/tools/lib/bpf/btf.h > >> +++ b/tools/lib/bpf/btf.h > >> @@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size); > >> LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset); > >> LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset); > >> LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); > >> +LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id); > > > > let's move this definition to after btf__parse() to keep all > > "constructors" together (we can move btf__get_from_id() there for > > completeness as well, I suppose). > > I thought about that but wasn't sure, OK will do. Ok, thanks. > > > > >> LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, > >> __u32 expected_key_size, > >> __u32 expected_value_size, > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index 242e97892043..eff005b1eba1 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) > >> { > >> struct bpf_prog_info_linear *info_linear; > >> struct bpf_prog_info *info; > >> - struct btf *btf = NULL; > >> int err = -EINVAL; > >> + struct btf *btf; > >> > >> info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0); > >> err = libbpf_get_error(info_linear); > >> @@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) > >> pr_warn("The target program doesn't have BTF\n"); > >> goto out; > >> } > >> - if (btf__get_from_id(info->btf_id, &btf)) { > >> + btf = btf__load_from_kernel_by_id(info->btf_id); > >> + if (libbpf_get_error(btf)) { > > > > there seems to be a bug in existing code and you are keeping it. On > > error err will be 0. Let's fix it. Same for above if (!info->btf_id), > > please fix that as well while you are at it. > > Oh right, I saw that err was initialised at -EINVAL and did not notice > it was changed for the info_linear. I'll address it. cool, thanks
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().
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().
2021-07-23 10:18 UTC-0700 ~ 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. OK, I think I was. I'm not arguing against the contract, but I thought your suggestion would introduce a change in btf__get_from_id()'s behaviour. Reading again through the code and your explanations, there should be no change indeed, I just misunderstood in the first place. Apologies, and thanks for your patience :). I'll prepare v3 soon. Quentin
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); + + btf = btf_get_from_fd(btf_fd, NULL); + close(btf_fd); + + return libbpf_ptr(btf); +} + int btf__get_from_id(__u32 id, struct btf **btf) { struct btf *res; - int err, btf_fd; + int err; *btf = NULL; - btf_fd = bpf_btf_get_fd_by_id(id); - if (btf_fd < 0) - return libbpf_err(-errno); - - res = btf_get_from_fd(btf_fd, NULL); + res = btf__load_from_kernel_by_id(id); err = libbpf_get_error(res); - close(btf_fd); - if (err) return libbpf_err(err); diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index fd8a21d936ef..3db9446bc133 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size); LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset); LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset); LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf); +LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id); LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, __u32 expected_key_size, __u32 expected_value_size, diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 242e97892043..eff005b1eba1 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) { struct bpf_prog_info_linear *info_linear; struct bpf_prog_info *info; - struct btf *btf = NULL; int err = -EINVAL; + struct btf *btf; info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0); err = libbpf_get_error(info_linear); @@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) pr_warn("The target program doesn't have BTF\n"); goto out; } - if (btf__get_from_id(info->btf_id, &btf)) { + btf = btf__load_from_kernel_by_id(info->btf_id); + if (libbpf_get_error(btf)) { pr_warn("Failed to get BTF of the program\n"); goto out; } diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index f7d52d76ca3a..ca8cc7a7faad 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -373,6 +373,7 @@ LIBBPF_0.5.0 { bpf_map__initial_value; bpf_map_lookup_and_delete_elem_flags; bpf_object__gen_loader; + btf__load_from_kernel_by_id; btf__load_into_kernel; btf_dump__dump_type_data; libbpf_set_strict_mode;