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

Commit Message

Grant Seltzer Richman Nov. 27, 2021, 9:02 p.m. UTC

From: Grant Seltzer <[e-mail]>

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 <[e-mail]>
---
 tools/lib/bpf/libbpf.h | 45 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Song Liu Nov. 27, 2021, 9:28 p.m. UTC | #1

On Sat, Nov 27, 2021 at 1:04 PM grantseltzer <[e-mail]> wrote:
>
> From: Grant Seltzer <[e-mail]>
>
> 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 <[e-mail]>

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
>

Andrii Nakryiko Nov. 29, 2021, 11:32 p.m. UTC | #2

On Sat, Nov 27, 2021 at 1:28 PM Song Liu <[e-mail]> wrote:
>
> On Sat, Nov 27, 2021 at 1:04 PM grantseltzer <[e-mail]> wrote:
> >
> > From: Grant Seltzer <[e-mail]>
> >
> > 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 <[e-mail]>
>
> 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
> >

Grant Seltzer Richman Dec. 6, 2021, 8:44 p.m. UTC | #3

On Mon, Nov 29, 2021 at 6:32 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Sat, Nov 27, 2021 at 1:28 PM Song Liu <[e-mail]> wrote:
> >
> > On Sat, Nov 27, 2021 at 1:04 PM grantseltzer <[e-mail]> wrote:
> > >
> > > From: Grant Seltzer <[e-mail]>
> > >
> > > 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 <[e-mail]>
> >
> > 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
> > >

Patch

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,