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