On Mon, Nov 29, 2021 at 6:32 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Sat, Nov 27, 2021 at 1:28 PM Song Liu <song@xxxxxxxxxx> wrote:
> >
> > On Sat, Nov 27, 2021 at 1:04 PM grantseltzer <grantseltzer@xxxxxxxxx> wrote:
> > >
> > > From: Grant Seltzer <grantseltzer@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> >
> > 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
> > >
