Message-ID: <731e94e1-b246-4838-9611-66d91d7d2518@fb.com>
Date:   Fri, 25 Sep 2020 08:37:47 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH bpf-next 7/9] libbpf: add BTF writing APIs

On 9/24/20 11:21 PM, Andrii Nakryiko wrote:
> On Thu, Sep 24, 2020 at 8:55 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
>>
>> On Wed, Sep 23, 2020 at 08:54:34AM -0700, Andrii Nakryiko wrote:
>>> Add APIs for appending new BTF types at the end of BTF object.
>>>
>>> Each BTF kind has either one API of the form btf__append_<kind>(). For types
>>> that have variable amount of additional items (struct/union, enum, func_proto,
>>> datasec), additional API is provided to emit each such item. E.g., for
>>> emitting a struct, one would use the following sequence of API calls:
>>>
>>> btf__append_struct(...);
>>> btf__append_field(...);
>>> ...
>>> btf__append_field(...);
>>
>> I've just started looking through the diffs. The first thing that struck me
>> is the name :) Why 'append' instead of 'add' ? The latter is shorter.
> 
> Append is very precise about those types being added at the end. Add
> is more ambiguous in that sense and doesn't imply any specific order.
> E.g., for btf__add_str() that's suitable, because the order in which
> strings are inserted might be different (e.g., if the string is
> duplicated). But it's not an "insert" either, so I'm fine with
> renaming to "add", if you prefer it.

The reason I prefer shorter is to be able to write:
btf__add_var(btf, "my_var", global,
              btf__add_const(btf,
              btf__add_volatile(btf,
              btf__add_ptr(btf,
              btf__add_int(btf, "int", 4, signed))));

In other words the shorter the type the more suitable the api
will be for manual construction of types.
Looks like the api already checks for invalid type_id,
so no need to check validity at every build stage.
Hence it's nice to combine multiple btf__add_*() into single line.

I think C language isn't great for 'constructor' style api.
May be on top of the above api we can add c++-like api?
For example we can define
struct btf_builder {
    struct btf_builder * (*_volatile) (void);
    struct btf_builder * (*_const) (void);
    struct btf_builder * (*_int) (char *name, int sz, int encoding);
    struct btf_builder * (_ptr) (void);
};

and the use it as:
     struct btf_builder *b = btf__create_global_builer(...);

     b->_int("int", 4, singed)
      ->_const()
      ->_volatile()
      ->_ptr()
      ->_var("my_var", global);

Every method will be return its own object (only one such object)
while the actual building will be happening in some 'invisible',
global, and mutex protected place.

>>
>> Also how would you add anon struct that is within another struct ?
>> The anon one would have to be added first and then added as a field?
>> Feels a bit odd that struct/union building doesn't have 'finish' method,
>> but I guess it can work.
> 
> That embedded anon struct will be a separate type, then the field
> (anonymous or not, that's orthogonal to anonymity of a struct (!))
> will refer to that anon struct type by its ID. Anon struct can be
> added right before, right after, or in between completely unrelated
> types, it doesn't matter to BTF itself as long as all the type IDs
> match in the end.
> 
> As for the finish method... There wasn't a need so far to have it, as
> BTF constantly maintains correct vlen for the "current"
> struct/union/func_proto/datasec/enum (anything with extra members).
> I've been writing a few more tests than what I posted here (they will
> come in the second wave of patches) and converted pahole to this new
> API completely. And it does feel pretty nice and natural so far. In
> the future we might add something like that, I suppose, that would
> allow to do some more validations at the end. But that would be a
> non-breaking extension, so I don't want to do it right now.

sure. that's fine.
Also I suspect sooner or later would be good to do at least partial
dedup of types while they're being built.
Instead of passing exact btf_id of 'int' everywhere it would be
human friendlier to say:
   b->_int("int", 4, singed)->_var("my_var")
instead of having extra variable that holds btf_id of 'int' and
use as it:
   u32 btf_id_of_int; /* that is used everywhere where 'int' field of 
var needs to be added */
   b->__var("my_var", btf_id_of_int);

I mean since types are being built the real dedup cannot happen,
but dedup of simple types can happen on the fly.
That will justify 'add' vs 'append' as well.
Just a thought.

Powered by blists - more mailing lists