On Mon, Sep 13, 2021 at 11:46 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Sep 9, 2021 at 1:43 PM grantseltzer <grantseltzer@xxxxxxxxx> wrote:
> >
> > From: Grant Seltzer <grantseltzer@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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
> >
