[PATCH bpf-next v2] Add sphinx code documentation comments

[PATCH bpf-next v2] Add sphinx code documentation comments

[grantseltzer]

From: Grant Seltzer <grantseltzer@gmail.com>

This adds comments above five functions in btf.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

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

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 4a711f990904..05e06f0136e3 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
 /* Copyright (c) 2018 Facebook */
+/*! \file */
 
 #ifndef __LIBBPF_BTF_H
 #define __LIBBPF_BTF_H
@@ -30,11 +31,47 @@ enum btf_endianness {
 	BTF_BIG_ENDIAN = 1,
 };
 
+/**
+ * @brief **btf__free()** frees all data of a BTF object.
+ * @param btf BTF object to free
+ * @return void
+ */
 LIBBPF_API void btf__free(struct btf *btf);
 
+/**
+ * @brief **btf__new()** creates a new instance of a BTF object.
+ * from the raw bytes of an ELF's BTF section
+ * @param data raw bytes
+ * @param size length of raw bytes
+ * @return new instance of BTF object which has to be eventually freed 
+ * with **btf__free()**
+ */
 LIBBPF_API struct btf *btf__new(const void *data, __u32 size);
+
+/**
+ * @brief **btf__new_split()** create a new instance of a BTF object from 
+ * the provided raw data bytes. It takes another BTF instance, **base_btf**, 
+ * which serves as a base BTF, which is extended by types in a newly created
+ * BTF instance.
+ * @param data raw bytes
+ * @param size length of raw bytes
+ * @param base_btf the base btf object
+ * @return struct btf *
+ */
 LIBBPF_API struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf);
+
+/**
+ * @brief **btf__new_empty()** creates an unpopulated BTF object.
+ * @return struct btf *
+ */
 LIBBPF_API struct btf *btf__new_empty(void);
+
+/**
+ * @brief **btf__new_empty_split()** creates an unpopulated
+ * BTF object from an ELF BTF section except with a base BTF
+ * on top of which split BTF should be based.
+ * @return struct btf *
+ */
 LIBBPF_API struct btf *btf__new_empty_split(struct btf *base_btf);
 
 LIBBPF_API struct btf *btf__parse(const char *path, struct btf_ext **btf_ext);
-- 
2.31.1

Re: [PATCH bpf-next v2] Add sphinx code documentation comments

[Andrii Nakryiko]

On Tue, Sep 14, 2021 at 1:17 PM grantseltzer <grantseltzer@gmail.com> wrote:
>
> From: Grant Seltzer <grantseltzer@gmail.com>
>
> This adds comments above five functions in btf.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
>
> Signed-off-by: Grant Seltzer <grantseltzer@gmail.com>
> ---
>  tools/lib/bpf/btf.h | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 4a711f990904..05e06f0136e3 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
>  /* Copyright (c) 2018 Facebook */
> +/*! \file */

What's the purpose of this? Is it some sort of description for the entire file?

>
>  #ifndef __LIBBPF_BTF_H
>  #define __LIBBPF_BTF_H
> @@ -30,11 +31,47 @@ enum btf_endianness {
>         BTF_BIG_ENDIAN = 1,
>  };
>
> +/**
> + * @brief **btf__free()** frees all data of a BTF object.
> + * @param btf BTF object to free
> + * @return void

agreed to drop this one

> + */
>  LIBBPF_API void btf__free(struct btf *btf);
>
> +/**
> + * @brief **btf__new()** creates a new instance of a BTF object.
> + * from the raw bytes of an ELF's BTF section
> + * @param data raw bytes
> + * @param size length of raw bytes

reads a bit weird, bytes don't have "length". "Number of bytes passed
in `data`"?

> + * @return new instance of BTF object which has to be eventually freed
> + * with **btf__free()**
> + */
>  LIBBPF_API struct btf *btf__new(const void *data, __u32 size);
> +
> +/**
> + * @brief **btf__new_split()** create a new instance of a BTF object from
> + * the provided raw data bytes. It takes another BTF instance, **base_btf**,
> + * which serves as a base BTF, which is extended by types in a newly created
> + * BTF instance.
> + * @param data raw bytes
> + * @param size length of raw bytes
> + * @param base_btf the base btf object
> + * @return struct btf *

I didn't think I had to leave a suggestion under every such empty @return...

BTW, return documentation is finally a good place where we should
document quirky libbpf error returning behavior. Something like this:

```
On error, error-code-encoded-as-pointer is returned, not a NULL. To
extract error code from such a pointer `libbpf_get_error()` should be
used. If `libbpf_set_strict_mode(LIBBPF_STRICT_CLEAN_PTRS)` is
enabled, NULL is returned on error instead. In both cases thread-local
`errno` variable is always set to error code as well.
```

We should have this remark under every pointer-returning API which has
this error-code-as-ptr logic (not all APIs do).


> + */
>  LIBBPF_API struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf);
> +
> +/**
> + * @brief **btf__new_empty()** creates an unpopulated BTF object.

We can add "Use `btf__add_*()` to populate such BTF object.

> + * @return struct btf *

another not described @return

> + */
>  LIBBPF_API struct btf *btf__new_empty(void);
> +
> +/**
> + * @brief **btf__new_empty_split()** creates an unpopulated
> + * BTF object from an ELF BTF section except with a base BTF
> + * on top of which split BTF should be based.

If *base_btf* is NULL, `btf__new_empty_split()` is equivalent to
`btf__new_empty()` and creates non-split BTF.

> + * @return struct btf *

and one more

> + */
>  LIBBPF_API struct btf *btf__new_empty_split(struct btf *base_btf);
>
>  LIBBPF_API struct btf *btf__parse(const char *path, struct btf_ext **btf_ext);
> --
> 2.31.1
>

Re: [PATCH bpf-next v2] Add sphinx code documentation comments

[Grant Seltzer Richman]

On Tue, Sep 14, 2021 at 8:12 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Sep 14, 2021 at 1:17 PM grantseltzer <grantseltzer@gmail.com> wrote:
> >
> > From: Grant Seltzer <grantseltzer@gmail.com>
> >
> > This adds comments above five functions in btf.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
> >
> > Signed-off-by: Grant Seltzer <grantseltzer@gmail.com>
> > ---
> >  tools/lib/bpf/btf.h | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > index 4a711f990904..05e06f0136e3 100644
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -1,5 +1,6 @@
> >  /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> >  /* Copyright (c) 2018 Facebook */
> > +/*! \file */
>
> What's the purpose of this? Is it some sort of description for the entire file?

Correct, we need to explicitly add the file to be tracked by doxygen
in order for function links to work.

>
> >
> >  #ifndef __LIBBPF_BTF_H
> >  #define __LIBBPF_BTF_H
> > @@ -30,11 +31,47 @@ enum btf_endianness {
> >         BTF_BIG_ENDIAN = 1,
> >  };
> >
> > +/**
> > + * @brief **btf__free()** frees all data of a BTF object.
> > + * @param btf BTF object to free
> > + * @return void
>
> agreed to drop this one
>
> > + */
> >  LIBBPF_API void btf__free(struct btf *btf);
> >
> > +/**
> > + * @brief **btf__new()** creates a new instance of a BTF object.
> > + * from the raw bytes of an ELF's BTF section
> > + * @param data raw bytes
> > + * @param size length of raw bytes
>
> reads a bit weird, bytes don't have "length". "Number of bytes passed
> in `data`"?
>
> > + * @return new instance of BTF object which has to be eventually freed
> > + * with **btf__free()**
> > + */
> >  LIBBPF_API struct btf *btf__new(const void *data, __u32 size);
> > +
> > +/**
> > + * @brief **btf__new_split()** create a new instance of a BTF object from
> > + * the provided raw data bytes. It takes another BTF instance, **base_btf**,
> > + * which serves as a base BTF, which is extended by types in a newly created
> > + * BTF instance.
> > + * @param data raw bytes
> > + * @param size length of raw bytes
> > + * @param base_btf the base btf object
> > + * @return struct btf *
>
> I didn't think I had to leave a suggestion under every such empty @return...
>
> BTW, return documentation is finally a good place where we should
> document quirky libbpf error returning behavior. Something like this:
>
> ```
> On error, error-code-encoded-as-pointer is returned, not a NULL. To
> extract error code from such a pointer `libbpf_get_error()` should be
> used. If `libbpf_set_strict_mode(LIBBPF_STRICT_CLEAN_PTRS)` is
> enabled, NULL is returned on error instead. In both cases thread-local
> `errno` variable is always set to error code as well.
> ```
>
> We should have this remark under every pointer-returning API which has
> this error-code-as-ptr logic (not all APIs do).
>
>
> > + */
> >  LIBBPF_API struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf);
> > +
> > +/**
> > + * @brief **btf__new_empty()** creates an unpopulated BTF object.
>
> We can add "Use `btf__add_*()` to populate such BTF object.
>
> > + * @return struct btf *
>
> another not described @return
>
> > + */
> >  LIBBPF_API struct btf *btf__new_empty(void);
> > +
> > +/**
> > + * @brief **btf__new_empty_split()** creates an unpopulated
> > + * BTF object from an ELF BTF section except with a base BTF
> > + * on top of which split BTF should be based.
>
> If *base_btf* is NULL, `btf__new_empty_split()` is equivalent to
> `btf__new_empty()` and creates non-split BTF.
>
> > + * @return struct btf *
>
> and one more
>
> > + */
> >  LIBBPF_API struct btf *btf__new_empty_split(struct btf *base_btf);
> >
> >  LIBBPF_API struct btf *btf__parse(const char *path, struct btf_ext **btf_ext);
> > --
> > 2.31.1
> >