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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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