On Tue, Sep 14, 2021 at 7:36 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Sep 14, 2021 at 12:52 PM Grant Seltzer Richman
> <grantseltzer@xxxxxxxxx> wrote:
> >
> > 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
>
> 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
> > > >
