[PATCH bpf-next] libbpf: Add doc comments in libb.h

[PATCH bpf-next] libbpf: Add doc comments in libb.h

[grantseltzer]

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
+ * 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);
 
-- 
2.31.1

Re: [PATCH bpf-next] libbpf: Add doc comments in libb.h

[Yonghong Song]

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

Re: [PATCH bpf-next] libbpf: Add doc comments in libb.h

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next] libbpf: Add doc comments in libb.h

[Grant Seltzer Richman]

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
> >

Re: [PATCH bpf-next] libbpf: Add doc comments in libb.h

[Grant Seltzer Richman]

On Mon, Nov 29, 2021 at 6:32 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Nov 27, 2021 at 1:28 PM Song Liu <song@kernel.org> wrote:
> >
> > On Sat, Nov 27, 2021 at 1:04 PM 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__open_file()
> > > - bpf_object__open_mem()
> > > - bpf_program__attach_uprobe()
> > > - bpf_program__attach_uprobe_opts()
> > >
> > > Signed-off-by: Grant Seltzer <grantseltzer@gmail.com>
> >
> > s/libb.h/libbpf.h/ in subject
> >
> > > ---
> > >  tools/lib/bpf/libbpf.h | 45 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 45 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 4ec69f224342..acfb207e71d1 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -108,8 +108,26 @@ struct bpf_object_open_opts {
> > >  #define bpf_object_open_opts__last_field btf_custom_path
> > >
> > >  LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
> > > +
> > > +/**
> > > + * @brief **bpf_object__open_file()** creates a bpf_object by opening
> > > + * the BPF object file pointed to by the passed path and loading it
>
> Would be nice to mention that it's ELF, no? "BPF ELF object file", maybe?
>
> > > + * into memory.
> > > + * @param path BPF object file relative or absolute path
>
> I started worrying about relative vs absolute paths after reading this
> :) I think just stating "BPF object file path" should be totally fine.
> Relative vs absolute works totally fine as expected with any API that
> accepts file paths.
>
>
> > > + * @param opts options for how to load the bpf object
>
> let's mention that opts are optional (i.e., you can pass NULL)
>
> > > + * @return pointer to the new bpf_object
> >
> > Please document return value on errors, i.e. libbpf_err_ptr(err)
> > instead of NULL. Same for all functions here.
>
> With libbpf 1.0 and forward APIs like this will return NULL on error,
> so let's use that as the convention in documentation. So something
> like "NULL is returned on error, error code is stored in errno"?
>
> >
> > > + */
> > >  LIBBPF_API struct bpf_object *
> > >  bpf_object__open_file(const char *path, const struct bpf_object_open_opts *opts);
> > > +
> > > +/**
> > > + * @brief **bpf_object__open_mem()** creates a bpf_object by reading
> > > + * the BPF objects raw bytes from an in memory buffer.
>
> typo: "an in"? Also it's even more important to mention that those
> bytes should be a valid BPF ELF object file?
>
> > > + * @param obj_buf pointer to the buffer containing bpf object bytes
>
> s/bpf object bytes/ELF file bytes/ ?
>
> > > + * @param obj_buf_sz number of bytes in the buffer
> > > + * @param opts options for how to load the bpf object
> > > + * @return pointer to the new bpf_object
> > > + */
> > >  LIBBPF_API struct bpf_object *
> > >  bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
> > >                      const struct bpf_object_open_opts *opts);
> > > @@ -344,10 +362,37 @@ struct bpf_uprobe_opts {
> > >  };
> > >  #define bpf_uprobe_opts__last_field retprobe
> > >
> > > +/**
> > > + * @brief **bpf_program__attach_uprobe** attaches a BPF program
>
> missing () after attach_uprobe
>
> > > + * to the userspace function which is found by binary path and
> > > + * offset. You can optionally specify a particular proccess to attach
> > s/proccess/process/
> >
> > > + * to. You can also optionally attach the program to the function
> > > + * exit instead of entry.
> > > + *
> > > + * @param prog BPF program to attach
> > > + * @param retprobe Attach to function exit
> > > + * @param pid Process ID to attach the uprobe to, -1 for all processes
>
> There is also 0 for self (own process).
>
> > > + * @param binary_path Path to binary that contains the function symbol
> > > + * @param func_offset Offset within the binary of the function symbol
> > > + * @return Reference to the newly created BPF link
>
> or NULL on error (errno is set to error code).
>
> > > + */
> > >  LIBBPF_API struct bpf_link *
> > >  bpf_program__attach_uprobe(const struct bpf_program *prog, bool retprobe,
> > >                            pid_t pid, const char *binary_path,
> > >                            size_t func_offset);
> > > +
> > > +/**
> > > + * @brief **bpf_program__attach_uprobe_opts** is just like
>
> () missing
>
> > > + * bpf_program__attach_uprobe except with a options struct
>
> (), let's use that around referenced to functions to make it clear
>
> > > + * for various configurations.
> > > + *
> > > + * @param prog BPF program to attach
> > > + * @param pid Process ID to attach the uprobe to, -1 for all processes
> > > + * @param binary_path Path to binary that contains the function symbol
> > > + * @param func_offset Offset within the binary of the function symbol
> > > + * @param opts Options for altering program attachment
> >
> > Let's also document details about these options.
>
> yep, but on uprobe_opts struct itself

I need to fix something with the configuration, the docs site
currently isn't displaying structs.

Also I haven't forgotten about adding a check for libbpf docs
generating properly. I'm working on a patch that I'll submit to the
docs tree allowing `make htmldocs` to take an argument for which
subsection to exclusively build.

>
>
> >
> > > + * @return Reference to the newly created BPF link
> > > + */
> > >  LIBBPF_API struct bpf_link *
> > >  bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> > >                                 const char *binary_path, size_t func_offset,
> > > --
> > > 2.31.1
> > >

Re: [PATCH bpf-next] libbpf: Add doc comments in libb.h

[Andrii Nakryiko]

On Sat, Nov 27, 2021 at 1:28 PM Song Liu <song@kernel.org> wrote:
>
> On Sat, Nov 27, 2021 at 1:04 PM 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__open_file()
> > - bpf_object__open_mem()
> > - bpf_program__attach_uprobe()
> > - bpf_program__attach_uprobe_opts()
> >
> > Signed-off-by: Grant Seltzer <grantseltzer@gmail.com>
>
> s/libb.h/libbpf.h/ in subject
>
> > ---
> >  tools/lib/bpf/libbpf.h | 45 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 4ec69f224342..acfb207e71d1 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -108,8 +108,26 @@ struct bpf_object_open_opts {
> >  #define bpf_object_open_opts__last_field btf_custom_path
> >
> >  LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
> > +
> > +/**
> > + * @brief **bpf_object__open_file()** creates a bpf_object by opening
> > + * the BPF object file pointed to by the passed path and loading it

Would be nice to mention that it's ELF, no? "BPF ELF object file", maybe?

> > + * into memory.
> > + * @param path BPF object file relative or absolute path

I started worrying about relative vs absolute paths after reading this
:) I think just stating "BPF object file path" should be totally fine.
Relative vs absolute works totally fine as expected with any API that
accepts file paths.


> > + * @param opts options for how to load the bpf object

let's mention that opts are optional (i.e., you can pass NULL)

> > + * @return pointer to the new bpf_object
>
> Please document return value on errors, i.e. libbpf_err_ptr(err)
> instead of NULL. Same for all functions here.

With libbpf 1.0 and forward APIs like this will return NULL on error,
so let's use that as the convention in documentation. So something
like "NULL is returned on error, error code is stored in errno"?

>
> > + */
> >  LIBBPF_API struct bpf_object *
> >  bpf_object__open_file(const char *path, const struct bpf_object_open_opts *opts);
> > +
> > +/**
> > + * @brief **bpf_object__open_mem()** creates a bpf_object by reading
> > + * the BPF objects raw bytes from an in memory buffer.

typo: "an in"? Also it's even more important to mention that those
bytes should be a valid BPF ELF object file?

> > + * @param obj_buf pointer to the buffer containing bpf object bytes

s/bpf object bytes/ELF file bytes/ ?

> > + * @param obj_buf_sz number of bytes in the buffer
> > + * @param opts options for how to load the bpf object
> > + * @return pointer to the new bpf_object
> > + */
> >  LIBBPF_API struct bpf_object *
> >  bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
> >                      const struct bpf_object_open_opts *opts);
> > @@ -344,10 +362,37 @@ struct bpf_uprobe_opts {
> >  };
> >  #define bpf_uprobe_opts__last_field retprobe
> >
> > +/**
> > + * @brief **bpf_program__attach_uprobe** attaches a BPF program

missing () after attach_uprobe

> > + * to the userspace function which is found by binary path and
> > + * offset. You can optionally specify a particular proccess to attach
> s/proccess/process/
>
> > + * to. You can also optionally attach the program to the function
> > + * exit instead of entry.
> > + *
> > + * @param prog BPF program to attach
> > + * @param retprobe Attach to function exit
> > + * @param pid Process ID to attach the uprobe to, -1 for all processes

There is also 0 for self (own process).

> > + * @param binary_path Path to binary that contains the function symbol
> > + * @param func_offset Offset within the binary of the function symbol
> > + * @return Reference to the newly created BPF link

or NULL on error (errno is set to error code).

> > + */
> >  LIBBPF_API struct bpf_link *
> >  bpf_program__attach_uprobe(const struct bpf_program *prog, bool retprobe,
> >                            pid_t pid, const char *binary_path,
> >                            size_t func_offset);
> > +
> > +/**
> > + * @brief **bpf_program__attach_uprobe_opts** is just like

() missing

> > + * bpf_program__attach_uprobe except with a options struct

(), let's use that around referenced to functions to make it clear

> > + * for various configurations.
> > + *
> > + * @param prog BPF program to attach
> > + * @param pid Process ID to attach the uprobe to, -1 for all processes
> > + * @param binary_path Path to binary that contains the function symbol
> > + * @param func_offset Offset within the binary of the function symbol
> > + * @param opts Options for altering program attachment
>
> Let's also document details about these options.

yep, but on uprobe_opts struct itself

>
> > + * @return Reference to the newly created BPF link
> > + */
> >  LIBBPF_API struct bpf_link *
> >  bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> >                                 const char *binary_path, size_t func_offset,
> > --
> > 2.31.1
> >

Re: [PATCH bpf-next] libbpf: Add doc comments in libb.h

[Song Liu]

On Sat, Nov 27, 2021 at 1:04 PM 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__open_file()
> - bpf_object__open_mem()
> - bpf_program__attach_uprobe()
> - bpf_program__attach_uprobe_opts()
>
> Signed-off-by: Grant Seltzer <grantseltzer@gmail.com>

s/libb.h/libbpf.h/ in subject

> ---
>  tools/lib/bpf/libbpf.h | 45 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 4ec69f224342..acfb207e71d1 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -108,8 +108,26 @@ struct bpf_object_open_opts {
>  #define bpf_object_open_opts__last_field btf_custom_path
>
>  LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
> +
> +/**
> + * @brief **bpf_object__open_file()** creates a bpf_object by opening
> + * the BPF object file pointed to by the passed path and loading it
> + * into memory.
> + * @param path BPF object file relative or absolute path
> + * @param opts options for how to load the bpf object
> + * @return pointer to the new bpf_object

Please document return value on errors, i.e. libbpf_err_ptr(err)
instead of NULL. Same for all functions here.

> + */
>  LIBBPF_API struct bpf_object *
>  bpf_object__open_file(const char *path, const struct bpf_object_open_opts *opts);
> +
> +/**
> + * @brief **bpf_object__open_mem()** creates a bpf_object by reading
> + * the BPF objects raw bytes from an in memory buffer.
> + * @param obj_buf pointer to the buffer containing bpf object bytes
> + * @param obj_buf_sz number of bytes in the buffer
> + * @param opts options for how to load the bpf object
> + * @return pointer to the new bpf_object
> + */
>  LIBBPF_API struct bpf_object *
>  bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
>                      const struct bpf_object_open_opts *opts);
> @@ -344,10 +362,37 @@ struct bpf_uprobe_opts {
>  };
>  #define bpf_uprobe_opts__last_field retprobe
>
> +/**
> + * @brief **bpf_program__attach_uprobe** attaches a BPF program
> + * to the userspace function which is found by binary path and
> + * offset. You can optionally specify a particular proccess to attach
s/proccess/process/

> + * to. You can also optionally attach the program to the function
> + * exit instead of entry.
> + *
> + * @param prog BPF program to attach
> + * @param retprobe Attach to function exit
> + * @param pid Process ID to attach the uprobe to, -1 for all processes
> + * @param binary_path Path to binary that contains the function symbol
> + * @param func_offset Offset within the binary of the function symbol
> + * @return Reference to the newly created BPF link
> + */
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_uprobe(const struct bpf_program *prog, bool retprobe,
>                            pid_t pid, const char *binary_path,
>                            size_t func_offset);
> +
> +/**
> + * @brief **bpf_program__attach_uprobe_opts** is just like
> + * bpf_program__attach_uprobe except with a options struct
> + * for various configurations.
> + *
> + * @param prog BPF program to attach
> + * @param pid Process ID to attach the uprobe to, -1 for all processes
> + * @param binary_path Path to binary that contains the function symbol
> + * @param func_offset Offset within the binary of the function symbol
> + * @param opts Options for altering program attachment

Let's also document details about these options.

> + * @return Reference to the newly created BPF link
> + */
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
>                                 const char *binary_path, size_t func_offset,
> --
> 2.31.1
>

[PATCH bpf-next] libbpf: Add doc comments in libb.h

[grantseltzer]

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__open_file()
- bpf_object__open_mem()
- bpf_program__attach_uprobe()
- bpf_program__attach_uprobe_opts()

Signed-off-by: Grant Seltzer <grantseltzer@gmail.com>
---
 tools/lib/bpf/libbpf.h | 45 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 4ec69f224342..acfb207e71d1 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -108,8 +108,26 @@ struct bpf_object_open_opts {
 #define bpf_object_open_opts__last_field btf_custom_path
 
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
+
+/**
+ * @brief **bpf_object__open_file()** creates a bpf_object by opening
+ * the BPF object file pointed to by the passed path and loading it
+ * into memory.
+ * @param path BPF object file relative or absolute path
+ * @param opts options for how to load the bpf object
+ * @return pointer to the new bpf_object
+ */
 LIBBPF_API struct bpf_object *
 bpf_object__open_file(const char *path, const struct bpf_object_open_opts *opts);
+
+/**
+ * @brief **bpf_object__open_mem()** creates a bpf_object by reading
+ * the BPF objects raw bytes from an in memory buffer.
+ * @param obj_buf pointer to the buffer containing bpf object bytes
+ * @param obj_buf_sz number of bytes in the buffer
+ * @param opts options for how to load the bpf object
+ * @return pointer to the new bpf_object
+ */
 LIBBPF_API struct bpf_object *
 bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
 		     const struct bpf_object_open_opts *opts);
@@ -344,10 +362,37 @@ struct bpf_uprobe_opts {
 };
 #define bpf_uprobe_opts__last_field retprobe
 
+/**
+ * @brief **bpf_program__attach_uprobe** attaches a BPF program
+ * to the userspace function which is found by binary path and 
+ * offset. You can optionally specify a particular proccess to attach
+ * to. You can also optionally attach the program to the function 
+ * exit instead of entry. 
+ *
+ * @param prog BPF program to attach
+ * @param retprobe Attach to function exit
+ * @param pid Process ID to attach the uprobe to, -1 for all processes
+ * @param binary_path Path to binary that contains the function symbol
+ * @param func_offset Offset within the binary of the function symbol
+ * @return Reference to the newly created BPF link
+ */
 LIBBPF_API struct bpf_link *
 bpf_program__attach_uprobe(const struct bpf_program *prog, bool retprobe,
 			   pid_t pid, const char *binary_path,
 			   size_t func_offset);
+
+/**
+ * @brief **bpf_program__attach_uprobe_opts** is just like 
+ * bpf_program__attach_uprobe except with a options struct
+ * for various configurations.
+ * 
+ * @param prog BPF program to attach
+ * @param pid Process ID to attach the uprobe to, -1 for all processes
+ * @param binary_path Path to binary that contains the function symbol
+ * @param func_offset Offset within the binary of the function symbol
+ * @param opts Options for altering program attachment
+ * @return Reference to the newly created BPF link 
+ */
 LIBBPF_API struct bpf_link *
 bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
 				const char *binary_path, size_t func_offset,
-- 
2.31.1