| Message ID | 20210917152300.13978-1-grantseltzer@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: Add doc comments in libb.h | 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 | 8 maintainers not CCed: kpsingh@kernel.org john.fastabend@gmail.com yhs@fb.com daniel@iogearbox.net songliubraving@fb.com netdev@vger.kernel.org ast@kernel.org kafai@fb.com |
| 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 | fail | Errors and warnings before: 0 this patch: 10 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: Possible repeated word: 'name' WARNING: line length of 81 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| bpf/vmtest-bpf-next | success | VM_Test |
| bpf/vmtest-bpf-next-PR | success | PR summary |
On 9/17/21 8:23 AM, grantseltzer wrote: > From: Grant Seltzer <grantseltzer@gmail.com> > > This adds comments above functions in libbpf.h which document > their uses. These comments are of a format that doxygen and sphinx > can pick up and render. These are rendered by libbpf.readthedocs.org A typo: subject line "libb.h" => "libbpf.h". > > These doc comments are for: > - bpf_object__find_map_by_name() > - bpf_map__fd() > - bpf_map__is_internal() > - libbpf_get_error() > - libbpf_num_possible_cpus() > > Signed-off-by: Grant Seltzer <grantseltzer@gmail.com> > --- > tools/lib/bpf/libbpf.h | 58 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 50 insertions(+), 8 deletions(-) > [...]
On Fri, Sep 17, 2021 at 8:25 AM grantseltzer <grantseltzer@gmail.com> wrote: > > From: Grant Seltzer <grantseltzer@gmail.com> > > This adds comments above functions in libbpf.h which document > their uses. These comments are of a format that doxygen and sphinx > can pick up and render. These are rendered by libbpf.readthedocs.org > > These doc comments are for: > - bpf_object__find_map_by_name() > - bpf_map__fd() > - bpf_map__is_internal() > - libbpf_get_error() > - libbpf_num_possible_cpus() > > Signed-off-by: Grant Seltzer <grantseltzer@gmail.com> > --- > tools/lib/bpf/libbpf.h | 58 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 50 insertions(+), 8 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 2f6f0e15d1e7..27a5ebf56d19 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -478,9 +478,14 @@ struct bpf_map_def { > unsigned int map_flags; > }; > > -/* > - * The 'struct bpf_map' in include/linux/bpf.h is internal to the kernel, > - * so no need to worry about a name clash. > +/** > + * @brief **bpf_object__find_map_by_name()** returns a pointer to the this whole "a pointer" wording seems very low-level. It's clear that it's a pointer just by looking at the function signature. Maybe let's use a slightly higher-level terminology when talking about bpf_map, bpf_program, bpf_object, etc. E.g., how about something like this: "... returns BPF map of the given name, if it exists within the passed BPF object." No need to describe what happens if it doesn't exist here, because we have @return section, which can be: "@return BPF map instance, if such map exists within BPF object; NULL, otherwise. > + * specified bpf map in the bpf object if that map exists, and returns let's use BPF (all caps) consistently throughout documentation > + * NULL if not. It sets errno in case of error. > + * @param obj bpf object > + * @param name name of the bpf map > + * @return the address of the map within the bpf object, or NULL if it > + * does not exist > */ > LIBBPF_API struct bpf_map * > bpf_object__find_map_by_name(const struct bpf_object *obj, const char *name); > @@ -506,7 +511,15 @@ bpf_map__next(const struct bpf_map *map, const struct bpf_object *obj); > LIBBPF_API struct bpf_map * > bpf_map__prev(const struct bpf_map *map, const struct bpf_object *obj); > > -/* get/set map FD */ > +/** > + * @brief **bpf_map__fd()** gets the file descriptor of the passed > + * bpf map > + * @param map the bpf map instance > + * @return the file descriptor or in case of an error, EINVAL or -EINVAL, not EINVAL > + * > + * errno should be checked after this call, it will be EINVAL in > + * case of error. this last part is misleading. -EINVAL is returned directly so you don't have to check errno. Let's maybe drop this sentence altogether, it's going to be extremely repetitive to specify that for each API. We should have a separate section about libbpf's approach to returning errors, for low-level/high-level int-returning APIs and pointer-returning APIs. > + */ > LIBBPF_API int bpf_map__fd(const struct bpf_map *map); > LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd); > /* get map definition */ > @@ -547,6 +560,15 @@ LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map, > const void *data, size_t size); > LIBBPF_API const void *bpf_map__initial_value(struct bpf_map *map, size_t *psize); > LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map); > + > +/** > + * @brief **bpf_map__is_internal()** tells the caller whether or not > + * the passed map is a special internal map let's expand a little bit: "a special map created by libbpf automatically for things like global variables, __ksym externs, Kconfig values, etc"? > + * @param map reference to the bpf_map > + * @return true if the map is an internal map, false if not s/if not/otherwise/ > + * > + * See the enum `libbpf_map_type` for listing of the types > + */ > LIBBPF_API bool bpf_map__is_internal(const struct bpf_map *map); > LIBBPF_API int bpf_map__set_pin_path(struct bpf_map *map, const char *path); > LIBBPF_API const char *bpf_map__get_pin_path(const struct bpf_map *map); > @@ -558,6 +580,24 @@ LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path); > LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd); > LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map); > > +/** > + * @brief **libbpf_get_error()** extracts the error code from the passed > + * pointer > + * @param ptr pointer returned from libbpf API function > + * @return error code "or 0, if no error happened" > + * > + * Many libbpf API functions which return pointers have logic to encode error > + * codes as pointers, and do not return NULL. Meaning **libbpf_get_error()** > + * should be used on the return value from these functions. Consult the > + * individual functions documentation to verify if this logic applies. > + * > + * For these API functions, if `libbpf_set_strict_mode(LIBBPF_STRICT_CLEAN_PTRS)` > + * is enabled, NULL is returned on error instead. > + * > + * If ptr == NULL, then errno should be already set by the failing nit: "*ptr* is NULL", it's not a code, it's docs for humans, so probably better to use conversational language here. Also do parameters have to be enclosed in *...*? Or it's just a typographic convention? Also, I think it's worth noting here that `libbpf_get_error()` has to be called right after the API itself with no other intervening calls that could modify errno. You might get away with this in non-strict mode, but in strict mode when pointer is NULL, the only way to get error code is through errno variable (which is what libbpf_get_error() is doing), but that assumes that errno is preserved. So probably worth stating "Use libbpf_get_error() to extract error immediately after calling an API function, with no intervening calls that could clobber `errno` variable." or something along those lines? > + * API, because libbpf never returns NULL on success and it now always > + * sets errno on error. > + */ > LIBBPF_API long libbpf_get_error(const void *ptr); > > struct bpf_prog_load_attr { > @@ -822,9 +862,12 @@ bpf_program__bpil_addr_to_offs(struct bpf_prog_info_linear *info_linear); > LIBBPF_API void > bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); > > -/* > - * A helper function to get the number of possible CPUs before looking up > - * per-CPU maps. Negative errno is returned on failure. > +/** > + * @brief **libbpf_num_possible_cpus()** is helper function to get the nit: is *a* helper function > + * number of possible CPUs before looking up per-CPU maps. "before looking up per-CPU maps" is too specific, it's not the only case. So let's keep it generic. It's the theoretically possible number of CPUs that a host kernel supports and expects. > + * @return number of possible CPUs > + * > + * Negative errno is returned on failure. errno is misleading here, is this about the thread-local errno variable or an error code? Let's use "error code" consistently throughout the docs. > * > * Example usage: > * > @@ -834,7 +877,6 @@ bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); > * } > * long values[ncpus]; > * bpf_map_lookup_elem(per_cpu_map_fd, key, values); > - * > */ > LIBBPF_API int libbpf_num_possible_cpus(void); > > -- > 2.31.1 >
On Fri, Sep 17, 2021 at 12:56 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Sep 17, 2021 at 8:25 AM grantseltzer <grantseltzer@gmail.com> wrote: > > > > From: Grant Seltzer <grantseltzer@gmail.com> > > > > This adds comments above functions in libbpf.h which document > > their uses. These comments are of a format that doxygen and sphinx > > can pick up and render. These are rendered by libbpf.readthedocs.org > > > > These doc comments are for: > > - bpf_object__find_map_by_name() > > - bpf_map__fd() > > - bpf_map__is_internal() > > - libbpf_get_error() > > - libbpf_num_possible_cpus() > > > > Signed-off-by: Grant Seltzer <grantseltzer@gmail.com> > > --- > > tools/lib/bpf/libbpf.h | 58 ++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 50 insertions(+), 8 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > index 2f6f0e15d1e7..27a5ebf56d19 100644 > > --- a/tools/lib/bpf/libbpf.h > > +++ b/tools/lib/bpf/libbpf.h > > @@ -478,9 +478,14 @@ struct bpf_map_def { > > unsigned int map_flags; > > }; > > > > -/* > > - * The 'struct bpf_map' in include/linux/bpf.h is internal to the kernel, > > - * so no need to worry about a name clash. > > +/** > > + * @brief **bpf_object__find_map_by_name()** returns a pointer to the > > this whole "a pointer" wording seems very low-level. It's clear that > it's a pointer just by looking at the function signature. Maybe let's > use a slightly higher-level terminology when talking about bpf_map, > bpf_program, bpf_object, etc. E.g., how about something like this: > > "... returns BPF map of the given name, if it exists within the passed > BPF object." No need to describe what happens if it doesn't exist > here, because we have @return section, which can be: > > "@return BPF map instance, if such map exists within BPF object; NULL, > otherwise. > > > + * specified bpf map in the bpf object if that map exists, and returns > > let's use BPF (all caps) consistently throughout documentation > > > + * NULL if not. It sets errno in case of error. > > + * @param obj bpf object > > + * @param name name of the bpf map > > + * @return the address of the map within the bpf object, or NULL if it > > + * does not exist > > */ > > LIBBPF_API struct bpf_map * > > bpf_object__find_map_by_name(const struct bpf_object *obj, const char *name); > > @@ -506,7 +511,15 @@ bpf_map__next(const struct bpf_map *map, const struct bpf_object *obj); > > LIBBPF_API struct bpf_map * > > bpf_map__prev(const struct bpf_map *map, const struct bpf_object *obj); > > > > -/* get/set map FD */ > > +/** > > + * @brief **bpf_map__fd()** gets the file descriptor of the passed > > + * bpf map > > + * @param map the bpf map instance > > + * @return the file descriptor or in case of an error, EINVAL > > or -EINVAL, not EINVAL > > > + * > > + * errno should be checked after this call, it will be EINVAL in > > + * case of error. > > this last part is misleading. -EINVAL is returned directly so you > don't have to check errno. Let's maybe drop this sentence altogether, > it's going to be extremely repetitive to specify that for each API. We > should have a separate section about libbpf's approach to returning > errors, for low-level/high-level int-returning APIs and > pointer-returning APIs. > > > + */ > > LIBBPF_API int bpf_map__fd(const struct bpf_map *map); > > LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd); > > /* get map definition */ > > @@ -547,6 +560,15 @@ LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map, > > const void *data, size_t size); > > LIBBPF_API const void *bpf_map__initial_value(struct bpf_map *map, size_t *psize); > > LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map); > > + > > +/** > > + * @brief **bpf_map__is_internal()** tells the caller whether or not > > + * the passed map is a special internal map > > let's expand a little bit: "a special map created by libbpf > automatically for things like global variables, __ksym externs, > Kconfig values, etc"? > > > + * @param map reference to the bpf_map > > + * @return true if the map is an internal map, false if not > > s/if not/otherwise/ > > > + * > > + * See the enum `libbpf_map_type` for listing of the types > > + */ > > LIBBPF_API bool bpf_map__is_internal(const struct bpf_map *map); > > LIBBPF_API int bpf_map__set_pin_path(struct bpf_map *map, const char *path); > > LIBBPF_API const char *bpf_map__get_pin_path(const struct bpf_map *map); > > @@ -558,6 +580,24 @@ LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path); > > LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd); > > LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map); > > > > +/** > > + * @brief **libbpf_get_error()** extracts the error code from the passed > > + * pointer > > + * @param ptr pointer returned from libbpf API function > > + * @return error code > > "or 0, if no error happened" > > > + * > > + * Many libbpf API functions which return pointers have logic to encode error > > + * codes as pointers, and do not return NULL. Meaning **libbpf_get_error()** > > + * should be used on the return value from these functions. Consult the > > + * individual functions documentation to verify if this logic applies. > > + * > > + * For these API functions, if `libbpf_set_strict_mode(LIBBPF_STRICT_CLEAN_PTRS)` > > + * is enabled, NULL is returned on error instead. > > + * > > + * If ptr == NULL, then errno should be already set by the failing > > nit: "*ptr* is NULL", it's not a code, it's docs for humans, so > probably better to use conversational language here. Also do > parameters have to be enclosed in *...*? Or it's just a typographic > convention? Purely convention. > > > Also, I think it's worth noting here that `libbpf_get_error()` has to > be called right after the API itself with no other intervening calls > that could modify errno. You might get away with this in non-strict > mode, but in strict mode when pointer is NULL, the only way to get > error code is through errno variable (which is what libbpf_get_error() > is doing), but that assumes that errno is preserved. So probably worth > stating "Use libbpf_get_error() to extract error immediately after > calling an API function, with no intervening calls that could clobber > `errno` variable." or something along those lines? > > > + * API, because libbpf never returns NULL on success and it now always > > + * sets errno on error. > > + */ > > LIBBPF_API long libbpf_get_error(const void *ptr); > > > > struct bpf_prog_load_attr { > > @@ -822,9 +862,12 @@ bpf_program__bpil_addr_to_offs(struct bpf_prog_info_linear *info_linear); > > LIBBPF_API void > > bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); > > > > -/* > > - * A helper function to get the number of possible CPUs before looking up > > - * per-CPU maps. Negative errno is returned on failure. > > +/** > > + * @brief **libbpf_num_possible_cpus()** is helper function to get the > > nit: is *a* helper function > > > + * number of possible CPUs before looking up per-CPU maps. > > "before looking up per-CPU maps" is too specific, it's not the only > case. So let's keep it generic. It's the theoretically possible number > of CPUs that a host kernel supports and expects. > > > + * @return number of possible CPUs > > + * > > + * Negative errno is returned on failure. > > errno is misleading here, is this about the thread-local errno > variable or an error code? Let's use "error code" consistently > throughout the docs. > > > * > > * Example usage: > > * > > @@ -834,7 +877,6 @@ bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); > > * } > > * long values[ncpus]; > > * bpf_map_lookup_elem(per_cpu_map_fd, key, values); > > - * > > */ > > LIBBPF_API int libbpf_num_possible_cpus(void); > > > > -- > > 2.31.1 > >
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 2f6f0e15d1e7..27a5ebf56d19 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -478,9 +478,14 @@ struct bpf_map_def { unsigned int map_flags; }; -/* - * The 'struct bpf_map' in include/linux/bpf.h is internal to the kernel, - * so no need to worry about a name clash. +/** + * @brief **bpf_object__find_map_by_name()** returns a pointer to the + * specified bpf map in the bpf object if that map exists, and returns + * NULL if not. It sets errno in case of error. + * @param obj bpf object + * @param name name of the bpf map + * @return the address of the map within the bpf object, or NULL if it + * does not exist */ LIBBPF_API struct bpf_map * bpf_object__find_map_by_name(const struct bpf_object *obj, const char *name); @@ -506,7 +511,15 @@ bpf_map__next(const struct bpf_map *map, const struct bpf_object *obj); LIBBPF_API struct bpf_map * bpf_map__prev(const struct bpf_map *map, const struct bpf_object *obj); -/* get/set map FD */ +/** + * @brief **bpf_map__fd()** gets the file descriptor of the passed + * bpf map + * @param map the bpf map instance + * @return the file descriptor or in case of an error, EINVAL + * + * errno should be checked after this call, it will be EINVAL in + * case of error. + */ LIBBPF_API int bpf_map__fd(const struct bpf_map *map); LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd); /* get map definition */ @@ -547,6 +560,15 @@ LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map, const void *data, size_t size); LIBBPF_API const void *bpf_map__initial_value(struct bpf_map *map, size_t *psize); LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map); + +/** + * @brief **bpf_map__is_internal()** tells the caller whether or not + * the passed map is a special internal map + * @param map reference to the bpf_map + * @return true if the map is an internal map, false if not + * + * See the enum `libbpf_map_type` for listing of the types + */ LIBBPF_API bool bpf_map__is_internal(const struct bpf_map *map); LIBBPF_API int bpf_map__set_pin_path(struct bpf_map *map, const char *path); LIBBPF_API const char *bpf_map__get_pin_path(const struct bpf_map *map); @@ -558,6 +580,24 @@ LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path); LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd); LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map); +/** + * @brief **libbpf_get_error()** extracts the error code from the passed + * pointer + * @param ptr pointer returned from libbpf API function + * @return error code + * + * Many libbpf API functions which return pointers have logic to encode error + * codes as pointers, and do not return NULL. Meaning **libbpf_get_error()** + * should be used on the return value from these functions. Consult the + * individual functions documentation to verify if this logic applies. + * + * For these API functions, if `libbpf_set_strict_mode(LIBBPF_STRICT_CLEAN_PTRS)` + * is enabled, NULL is returned on error instead. + * + * If ptr == NULL, then errno should be already set by the failing + * API, because libbpf never returns NULL on success and it now always + * sets errno on error. + */ LIBBPF_API long libbpf_get_error(const void *ptr); struct bpf_prog_load_attr { @@ -822,9 +862,12 @@ bpf_program__bpil_addr_to_offs(struct bpf_prog_info_linear *info_linear); LIBBPF_API void bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); -/* - * A helper function to get the number of possible CPUs before looking up - * per-CPU maps. Negative errno is returned on failure. +/** + * @brief **libbpf_num_possible_cpus()** is helper function to get the + * number of possible CPUs before looking up per-CPU maps. + * @return number of possible CPUs + * + * Negative errno is returned on failure. * * Example usage: * @@ -834,7 +877,6 @@ bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); * } * long values[ncpus]; * bpf_map_lookup_elem(per_cpu_map_fd, key, values); - * */ LIBBPF_API int libbpf_num_possible_cpus(void);