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

[PATCH bpf-next] libbpf: 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 | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 4a711f990904..f928e57c238c 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -30,11 +30,47 @@ enum btf_endianness {
 	BTF_BIG_ENDIAN = 1,
 };
 
+/**
+ * @brief **btf__free** frees all data of the BTF representation
+ * @param btf
+ * @return void
+ */
 LIBBPF_API void btf__free(struct btf *btf);
 
+/**
+ * @brief **btf__new** creates a representation of a BTF section
+ * (struct btf) from the raw bytes of that section
+ * @param data raw bytes
+ * @param size length of raw bytes
+ * @return struct btf*
+ */
 LIBBPF_API struct btf *btf__new(const void *data, __u32 size);
+
+/**
+ * @brief **btf__new_split** creates a representation of a BTF section
+ * (struct btf) from a combination of raw bytes and a btf struct
+ * where the btf struct provides a basic set of types and strings,
+ * while the raw data adds its own new types and strings
+ * @param data raw bytes
+ * @param size length of raw bytes
+ * @param base_btf the base btf representation
+ * @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 representation of
+ * a BTF section
+ * @return struct btf*
+ */
 LIBBPF_API struct btf *btf__new_empty(void);
+
+/**
+ * @brief **btf__new_empty_split** creates an unpopulated
+ * representation of a BTF section except with a base BTF
+ * ontop of which split BTF should be based
+ * @return struct btf*q
+ */
 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] libbpf: Add sphinx code documentation comments

[Grant Seltzer Richman]

On Thu, Sep 9, 2021 at 4:43 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 | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 4a711f990904..f928e57c238c 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -30,11 +30,47 @@ enum btf_endianness {
>         BTF_BIG_ENDIAN = 1,
>  };
>
> +/**
> + * @brief **btf__free** frees all data of the BTF representation
> + * @param btf
> + * @return void
> + */
>  LIBBPF_API void btf__free(struct btf *btf);
>
> +/**
> + * @brief **btf__new** creates a representation of a BTF section
> + * (struct btf) from the raw bytes of that section
> + * @param data raw bytes
> + * @param size length of raw bytes
> + * @return struct btf*
> + */
>  LIBBPF_API struct btf *btf__new(const void *data, __u32 size);
> +
> +/**
> + * @brief **btf__new_split** creates a representation of a BTF section
> + * (struct btf) from a combination of raw bytes and a btf struct
> + * where the btf struct provides a basic set of types and strings,
> + * while the raw data adds its own new types and strings
> + * @param data raw bytes
> + * @param size length of raw bytes
> + * @param base_btf the base btf representation
> + * @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 representation of
> + * a BTF section
> + * @return struct btf*
> + */
>  LIBBPF_API struct btf *btf__new_empty(void);
> +
> +/**
> + * @brief **btf__new_empty_split** creates an unpopulated
> + * representation of a BTF section except with a base BTF
> + * ontop of which split BTF should be based
> + * @return struct btf*q
> + */
>  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


I should note that you can view what this actually looks like here:
https://libbpf-test.readthedocs.io/en/latest/api.html

Also I would use this (plus your feedback) to make a document laying
out a standard convention for formatting these doc comments and
contribute that via the github mirror.

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

[Andrii Nakryiko]

On Thu, Sep 9, 2021 at 1:43 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 | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>

It's nice that you provided a test instance of readthedocs.io site, it
made it much easier to look at how all this is rendered. Thanks!

I'm no technical writer, but left some thoughts below. It would be
great to get more help documenting all the APIs and important libbpf
objects from people that are good at succinctly explaining concepts.
This is a great start!

> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 4a711f990904..f928e57c238c 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -30,11 +30,47 @@ enum btf_endianness {
>         BTF_BIG_ENDIAN = 1,
>  };
>
> +/**
> + * @brief **btf__free** frees all data of the BTF representation

I'd like to propose that we add () for all functions, I've been
roughly following this convention throughout commit messages and
comments in the code, I think it's makes it a bit clearer that we are
talking about functions

> + * @param btf

seems like the format is "<arg_name> <description of the argument>" so
in this case it should be something like

@param btf BTF object to free

> + * @return void

do we need @return if the function is returning void? What if we just
omit @return for such cases?

> + */
>  LIBBPF_API void btf__free(struct btf *btf);
>
> +/**
> + * @brief **btf__new** creates a representation of a BTF section
> + * (struct btf) from the raw bytes of that section

is there some way to cross reference to other types/functions? E.g.,
how do we make `struct btf` a link to its description?

> + * @param data raw bytes
> + * @param size length of raw bytes
> + * @return struct btf*


@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** creates a representation of a BTF section

"representation of a BTF section" seems a bit mouthful. And it's not a
representation, it's a BTF object that allows to perform a lot of
stuff with BTF type information. So maybe let's describe it as

**btf__new_split()** create a new instance of BTF object from 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.

> + * (struct btf) from a combination of raw bytes and a btf struct
> + * where the btf struct provides a basic set of types and strings,
> + * while the raw data adds its own new types and strings
> + * @param data raw bytes
> + * @param size length of raw bytes
> + * @param base_btf the base btf representation

same here, "representation" sounds kind of weird and wrong here

> + * @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 representation of
> + * a BTF section
> + * @return struct btf*
> + */
>  LIBBPF_API struct btf *btf__new_empty(void);
> +
> +/**
> + * @brief **btf__new_empty_split** creates an unpopulated
> + * representation of a BTF section except with a base BTF
> + * ontop of which split BTF should be based

typo: on top

> + * @return struct btf*q
> + */
>  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] libbpf: Add sphinx code documentation comments

[Grant Seltzer Richman]

On Mon, Sep 13, 2021 at 11:46 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Sep 9, 2021 at 1:43 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 | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
>
> It's nice that you provided a test instance of readthedocs.io site, it
> made it much easier to look at how all this is rendered. Thanks!
>
> I'm no technical writer, but left some thoughts below. It would be
> great to get more help documenting all the APIs and important libbpf
> objects from people that are good at succinctly explaining concepts.

For sure! Once we have these doc comments we can point to them as
examples. I'm going to write a blog post and advertise to people to
see how they can contribute.

> This is a great start!
>
> > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > index 4a711f990904..f928e57c238c 100644
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -30,11 +30,47 @@ enum btf_endianness {
> >         BTF_BIG_ENDIAN = 1,
> >  };
> >
> > +/**
> > + * @brief **btf__free** frees all data of the BTF representation
>
> I'd like to propose that we add () for all functions, I've been
> roughly following this convention throughout commit messages and
> comments in the code, I think it's makes it a bit clearer that we are
> talking about functions
>
> > + * @param btf
>
> seems like the format is "<arg_name> <description of the argument>" so
> in this case it should be something like
>
> @param btf BTF object to free
>
> > + * @return void
>
> do we need @return if the function is returning void? What if we just
> omit @return for such cases?
>
> > + */
> >  LIBBPF_API void btf__free(struct btf *btf);
> >
> > +/**
> > + * @brief **btf__new** creates a representation of a BTF section
> > + * (struct btf) from the raw bytes of that section
>
> is there some way to cross reference to other types/functions? E.g.,
> how do we make `struct btf` a link to its description?

Yes we can cross reference against other members that are in the
generated documentation (my v2 patch will have this), but not ones
that aren't present. `struct btf` is not because it's in btf.c and not
btf.h. It's a little messy to have documentation from both as it leads
to some generation of non-API functions and duplication. One way of
getting around that mess is to explicitly name
functions/variables/defines/types that we want to have in
documentation but that's not automatically maintained.

For this example, is there a reason that `struct btf` is defined in
btf.c and not btf.h? Would it be possible to have all struct
definitions in header files?

>
> > + * @param data raw bytes
> > + * @param size length of raw bytes
> > + * @return struct btf*
>
>
> @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** creates a representation of a BTF section
>
> "representation of a BTF section" seems a bit mouthful. And it's not a
> representation, it's a BTF object that allows to perform a lot of
> stuff with BTF type information. So maybe let's describe it as
>
> **btf__new_split()** create a new instance of BTF object from 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.
>
> > + * (struct btf) from a combination of raw bytes and a btf struct
> > + * where the btf struct provides a basic set of types and strings,
> > + * while the raw data adds its own new types and strings
> > + * @param data raw bytes
> > + * @param size length of raw bytes
> > + * @param base_btf the base btf representation
>
> same here, "representation" sounds kind of weird and wrong here
>
> > + * @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 representation of
> > + * a BTF section
> > + * @return struct btf*
> > + */
> >  LIBBPF_API struct btf *btf__new_empty(void);
> > +
> > +/**
> > + * @brief **btf__new_empty_split** creates an unpopulated
> > + * representation of a BTF section except with a base BTF
> > + * ontop of which split BTF should be based
>
> typo: on top
>
> > + * @return struct btf*q
> > + */
> >  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] libbpf: Add sphinx code documentation comments

[Grant Seltzer Richman]

On Mon, Sep 13, 2021 at 11:46 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Sep 9, 2021 at 1:43 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 | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
>
> It's nice that you provided a test instance of readthedocs.io site, it
> made it much easier to look at how all this is rendered. Thanks!
>
> I'm no technical writer, but left some thoughts below. It would be
> great to get more help documenting all the APIs and important libbpf
> objects from people that are good at succinctly explaining concepts.
> This is a great start!
>
> > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > index 4a711f990904..f928e57c238c 100644
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -30,11 +30,47 @@ enum btf_endianness {
> >         BTF_BIG_ENDIAN = 1,
> >  };
> >
> > +/**
> > + * @brief **btf__free** frees all data of the BTF representation
>
> I'd like to propose that we add () for all functions, I've been
> roughly following this convention throughout commit messages and
> comments in the code, I think it's makes it a bit clearer that we are
> talking about functions
>
> > + * @param btf
>
> seems like the format is "<arg_name> <description of the argument>" so
> in this case it should be something like
>
> @param btf BTF object to free
>
> > + * @return void
>
> do we need @return if the function is returning void? What if we just
> omit @return for such cases?

Forgot to address this in the previous email. We certainly can
eliminate this line, and it would just not render anything for the
return. I'm in favor of keeping it explicit but certainly can remove
it if you'd like!

>
> > + */
> >  LIBBPF_API void btf__free(struct btf *btf);
> >
> > +/**
> > + * @brief **btf__new** creates a representation of a BTF section
> > + * (struct btf) from the raw bytes of that section
>
> is there some way to cross reference to other types/functions? E.g.,
> how do we make `struct btf` a link to its description?
>
> > + * @param data raw bytes
> > + * @param size length of raw bytes
> > + * @return struct btf*
>
>
> @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** creates a representation of a BTF section
>
> "representation of a BTF section" seems a bit mouthful. And it's not a
> representation, it's a BTF object that allows to perform a lot of
> stuff with BTF type information. So maybe let's describe it as
>
> **btf__new_split()** create a new instance of BTF object from 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.
>
> > + * (struct btf) from a combination of raw bytes and a btf struct
> > + * where the btf struct provides a basic set of types and strings,
> > + * while the raw data adds its own new types and strings
> > + * @param data raw bytes
> > + * @param size length of raw bytes
> > + * @param base_btf the base btf representation
>
> same here, "representation" sounds kind of weird and wrong here
>
> > + * @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 representation of
> > + * a BTF section
> > + * @return struct btf*
> > + */
> >  LIBBPF_API struct btf *btf__new_empty(void);
> > +
> > +/**
> > + * @brief **btf__new_empty_split** creates an unpopulated
> > + * representation of a BTF section except with a base BTF
> > + * ontop of which split BTF should be based
>
> typo: on top
>
> > + * @return struct btf*q
> > + */
> >  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] libbpf: Add sphinx code documentation comments

[Andrii Nakryiko]

On Tue, Sep 14, 2021 at 12:52 PM Grant Seltzer Richman
<grantseltzer@gmail.com> wrote:
>
> On Mon, Sep 13, 2021 at 11:46 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Sep 9, 2021 at 1:43 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 | 36 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >
> >
> > It's nice that you provided a test instance of readthedocs.io site, it
> > made it much easier to look at how all this is rendered. Thanks!
> >
> > I'm no technical writer, but left some thoughts below. It would be
> > great to get more help documenting all the APIs and important libbpf
> > objects from people that are good at succinctly explaining concepts.
>
> For sure! Once we have these doc comments we can point to them as
> examples. I'm going to write a blog post and advertise to people to
> see how they can contribute.
>
> > This is a great start!
> >
> > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > > index 4a711f990904..f928e57c238c 100644
> > > --- a/tools/lib/bpf/btf.h
> > > +++ b/tools/lib/bpf/btf.h
> > > @@ -30,11 +30,47 @@ enum btf_endianness {
> > >         BTF_BIG_ENDIAN = 1,
> > >  };
> > >
> > > +/**
> > > + * @brief **btf__free** frees all data of the BTF representation
> >
> > I'd like to propose that we add () for all functions, I've been
> > roughly following this convention throughout commit messages and
> > comments in the code, I think it's makes it a bit clearer that we are
> > talking about functions
> >
> > > + * @param btf
> >
> > seems like the format is "<arg_name> <description of the argument>" so
> > in this case it should be something like
> >
> > @param btf BTF object to free
> >
> > > + * @return void
> >
> > do we need @return if the function is returning void? What if we just
> > omit @return for such cases?
> >
> > > + */
> > >  LIBBPF_API void btf__free(struct btf *btf);
> > >
> > > +/**
> > > + * @brief **btf__new** creates a representation of a BTF section
> > > + * (struct btf) from the raw bytes of that section
> >
> > is there some way to cross reference to other types/functions? E.g.,
> > how do we make `struct btf` a link to its description?
>
> Yes we can cross reference against other members that are in the
> generated documentation (my v2 patch will have this), but not ones
> that aren't present. `struct btf` is not because it's in btf.c and not

I see. If it's impossible to jump to forward reference declaration,
that's fine, I suppose. We don't have to get this perfect from the
first try.

> btf.h. It's a little messy to have documentation from both as it leads
> to some generation of non-API functions and duplication. One way of
> getting around that mess is to explicitly name
> functions/variables/defines/types that we want to have in
> documentation but that's not automatically maintained.
>
> For this example, is there a reason that `struct btf` is defined in

yes, struct btf internals (actual fields and their layout) is internal
libbpf implementation detail and not part of its API. So public
headers only define forward references. If possible, it's good to add
brief comments to such forward references (similarly we have
bpf_program, bpf_map, bpf_object, btf_dump, etc), specifying what is
their purpose. But if not, we can have a separate page going over main
"objects" that libbpf API defines (bpf_object, bpf_map, bpf_program,
bpf_link, btf, btf_ext, btf_dump, probably some more I'm forgetting).
All those have hidden internals, but they are themselves very visible
in the API.

> btf.c and not btf.h? Would it be possible to have all struct
> definitions in header files?
>
> >
> > > + * @param data raw bytes
> > > + * @param size length of raw bytes
> > > + * @return struct btf*
> >
> >
> > @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** creates a representation of a BTF section
> >
> > "representation of a BTF section" seems a bit mouthful. And it's not a
> > representation, it's a BTF object that allows to perform a lot of
> > stuff with BTF type information. So maybe let's describe it as
> >
> > **btf__new_split()** create a new instance of BTF object from 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.
> >
> > > + * (struct btf) from a combination of raw bytes and a btf struct
> > > + * where the btf struct provides a basic set of types and strings,
> > > + * while the raw data adds its own new types and strings
> > > + * @param data raw bytes
> > > + * @param size length of raw bytes
> > > + * @param base_btf the base btf representation
> >
> > same here, "representation" sounds kind of weird and wrong here
> >
> > > + * @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 representation of
> > > + * a BTF section
> > > + * @return struct btf*
> > > + */
> > >  LIBBPF_API struct btf *btf__new_empty(void);
> > > +
> > > +/**
> > > + * @brief **btf__new_empty_split** creates an unpopulated
> > > + * representation of a BTF section except with a base BTF
> > > + * ontop of which split BTF should be based
> >
> > typo: on top
> >
> > > + * @return struct btf*q
> > > + */
> > >  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] libbpf: Add sphinx code documentation comments

[Grant Seltzer Richman]

On Tue, Sep 14, 2021 at 7:36 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Sep 14, 2021 at 12:52 PM Grant Seltzer Richman
> <grantseltzer@gmail.com> wrote:
> >
> > On Mon, Sep 13, 2021 at 11:46 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Thu, Sep 9, 2021 at 1:43 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 | 36 ++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 36 insertions(+)
> > > >
> > >
> > > It's nice that you provided a test instance of readthedocs.io site, it
> > > made it much easier to look at how all this is rendered. Thanks!
> > >
> > > I'm no technical writer, but left some thoughts below. It would be
> > > great to get more help documenting all the APIs and important libbpf
> > > objects from people that are good at succinctly explaining concepts.
> >
> > For sure! Once we have these doc comments we can point to them as
> > examples. I'm going to write a blog post and advertise to people to
> > see how they can contribute.
> >
> > > This is a great start!
> > >
> > > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > > > index 4a711f990904..f928e57c238c 100644
> > > > --- a/tools/lib/bpf/btf.h
> > > > +++ b/tools/lib/bpf/btf.h
> > > > @@ -30,11 +30,47 @@ enum btf_endianness {
> > > >         BTF_BIG_ENDIAN = 1,
> > > >  };
> > > >
> > > > +/**
> > > > + * @brief **btf__free** frees all data of the BTF representation
> > >
> > > I'd like to propose that we add () for all functions, I've been
> > > roughly following this convention throughout commit messages and
> > > comments in the code, I think it's makes it a bit clearer that we are
> > > talking about functions
> > >
> > > > + * @param btf
> > >
> > > seems like the format is "<arg_name> <description of the argument>" so
> > > in this case it should be something like
> > >
> > > @param btf BTF object to free
> > >
> > > > + * @return void
> > >
> > > do we need @return if the function is returning void? What if we just
> > > omit @return for such cases?
> > >
> > > > + */
> > > >  LIBBPF_API void btf__free(struct btf *btf);
> > > >
> > > > +/**
> > > > + * @brief **btf__new** creates a representation of a BTF section
> > > > + * (struct btf) from the raw bytes of that section
> > >
> > > is there some way to cross reference to other types/functions? E.g.,
> > > how do we make `struct btf` a link to its description?
> >
> > Yes we can cross reference against other members that are in the
> > generated documentation (my v2 patch will have this), but not ones
> > that aren't present. `struct btf` is not because it's in btf.c and not
>
> I see. If it's impossible to jump to forward reference declaration,
> that's fine, I suppose. We don't have to get this perfect from the
> first try.

There must be a way to do this, I will continue to experiment/research.

>
> > btf.h. It's a little messy to have documentation from both as it leads
> > to some generation of non-API functions and duplication. One way of
> > getting around that mess is to explicitly name
> > functions/variables/defines/types that we want to have in
> > documentation but that's not automatically maintained.
> >
> > For this example, is there a reason that `struct btf` is defined in
>
> yes, struct btf internals (actual fields and their layout) is internal
> libbpf implementation detail and not part of its API. So public
> headers only define forward references. If possible, it's good to add
> brief comments to such forward references (similarly we have
> bpf_program, bpf_map, bpf_object, btf_dump, etc), specifying what is
> their purpose. But if not, we can have a separate page going over main
> "objects" that libbpf API defines (bpf_object, bpf_map, bpf_program,
> bpf_link, btf, btf_ext, btf_dump, probably some more I'm forgetting).
> All those have hidden internals, but they are themselves very visible
> in the API.
>
> > btf.c and not btf.h? Would it be possible to have all struct
> > definitions in header files?
> >
> > >
> > > > + * @param data raw bytes
> > > > + * @param size length of raw bytes
> > > > + * @return struct btf*
> > >
> > >
> > > @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** creates a representation of a BTF section
> > >
> > > "representation of a BTF section" seems a bit mouthful. And it's not a
> > > representation, it's a BTF object that allows to perform a lot of
> > > stuff with BTF type information. So maybe let's describe it as
> > >
> > > **btf__new_split()** create a new instance of BTF object from 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.
> > >
> > > > + * (struct btf) from a combination of raw bytes and a btf struct
> > > > + * where the btf struct provides a basic set of types and strings,
> > > > + * while the raw data adds its own new types and strings
> > > > + * @param data raw bytes
> > > > + * @param size length of raw bytes
> > > > + * @param base_btf the base btf representation
> > >
> > > same here, "representation" sounds kind of weird and wrong here
> > >
> > > > + * @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 representation of
> > > > + * a BTF section
> > > > + * @return struct btf*
> > > > + */
> > > >  LIBBPF_API struct btf *btf__new_empty(void);
> > > > +
> > > > +/**
> > > > + * @brief **btf__new_empty_split** creates an unpopulated
> > > > + * representation of a BTF section except with a base BTF
> > > > + * ontop of which split BTF should be based
> > >
> > > typo: on top
> > >
> > > > + * @return struct btf*q
> > > > + */
> > > >  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
> > > >