Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
> On Tue, Oct 1, 2019 at 1:42 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>
>> Andrii Nakryiko <andriin@xxxxxx> writes:
>>
>> > Add new set of bpf_object__open APIs using new approach to optional
>> > parameters extensibility allowing simpler ABI compatibility approach.
>> >
>> > This patch demonstrates an approach to implementing libbpf APIs that
>> > makes it easy to extend existing APIs with extra optional parameters in
>> > such a way, that ABI compatibility is preserved without having to do
>> > symbol versioning and generating lots of boilerplate code to handle it.
>> > To facilitate succinct code for working with options, add OPTS_VALID,
>> > OPTS_HAS, and OPTS_GET macros that hide all the NULL and size checks.
>> >
>> > Additionally, newly added libbpf APIs are encouraged to follow similar
>> > pattern of having all mandatory parameters as formal function parameters
>> > and always have optional (NULL-able) xxx_opts struct, which should
>> > always have real struct size as a first field and the rest would be
>> > optional parameters added over time, which tune the behavior of existing
>> > API, if specified by user.
>>
>> I think this is a reasonable idea. It does require some care when adding
>> new options, though. They have to be truly optional. I.e., I could
>> imagine that we will have cases where the behaviour might need to be
>> different if a program doesn't understand a particular option (I am
>> working on such a case in the kernel ATM). You could conceivably use the
>> OPTS_HAS() macro to test for this case in the code, but that breaks if a
>> program is recompiled with no functional change: then it would *appear*
>> to "understand" that option, but not react properly to it.
>
> So let me double-check I'm understanding this correctly.
>
> Let's say we have some initial options like:
>
> // VERSION 1
> struct bla_opts {
> size_t sz;
> };
>
> // VERSION 2
> Then in newer version we add new field:
> struct bla_opts {
> int awesomeness_trigger;
> };
>
> Are you saying that if program was built with VERSION 1 in mind (so sz
> = 8 for bla_opts, so awesomeness_trigger can't be even specified),
> then that should be different from the program built against VERSION 2
> and specifying .awesomeness_trigger = 0?
> Do I get this right? I'm not sure how to otherwise interpret what you
> are saying, so please elaborate if I didn't get the idea.
>
> If that's what you are saying, then I think we shouldn't (and we
> really can't, see Jesper's remark about padding) distinguish between
> whether field was not "physically" there or whether it was just set to
> default 0 value. Treating this uniformly as 0 makes libbpf logic
> simpler and consistent and behavior much less surprising.
Indeed. My point was that we should make sure we don't try to do this :)
>> In other words, this should only be used for truly optional bits (like
>> flags) where the default corresponds to unchanged behaviour relative to
>> when the option was added.
>
> This I agree 100%, furthermore, any added new option has to behave
> like this. If that's not the case, then it has to be a new API
> function or at least another symbol version.
Exactly!
>>
>> A few comments on the syntax below...
>>
>>
>> > +static struct bpf_object *
>> > +__bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
>> > + struct bpf_object_open_opts *opts, bool enforce_kver)
>>
>> I realise this is an internal function, but why does it have a
>> non-optional parameter *after* the opts?
>
> Oh, no reason, added it later and I'm hoping to remove it completely.
> Current bpf_object__open_buffer always enforces kver presence in a
> program, which differs from bpf_object__open behavior (where it
> depends on provided .prog_type argument), so with this I tried to
> preserve existing behavior. But in the final version of this patch I
> think I'll just make this kver archaic business in libbpf not
> enforced. It's been deleted from kernel long time ago, there is no
> good reason to keep enforcing this in libbpf. If someone is running
> against old kernel and didn't specify kver, they'll get error anyway.
> Libbpf will just need to make sure to pass kver through, if it's
> specified. Thoughts?
Not many. Enforcing anything on kernel version seems brittle anyway, so
off the top of my head, yeah, let's nuke it (in a backwards-compatible
way, of course :)).
>>
>> > char tmp_name[64];
>> > + const char *name;
>> >
>> > - /* param validation */
>> > - if (!obj_buf || obj_buf_sz <= 0)
>> > - return NULL;
>> > + if (!OPTS_VALID(opts) || !obj_buf || obj_buf_sz == 0)
>> > + return ERR_PTR(-EINVAL);
>> >
>> > + name = OPTS_GET(opts, object_name, NULL);
>> > if (!name) {
>> > snprintf(tmp_name, sizeof(tmp_name), "%lx-%lx",
>> > (unsigned long)obj_buf,
>> > (unsigned long)obj_buf_sz);
>> > name = tmp_name;
>> > }
>> > +
>> > pr_debug("loading object '%s' from buffer\n", name);
>> >
>> > - return __bpf_object__open(name, obj_buf, obj_buf_sz, true, true);
>> > + return __bpf_object__open(name, obj_buf, obj_buf_sz, enforce_kver, 0);
>> > +}
>> > +
>> > +struct bpf_object *
>> > +bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
>> > + struct bpf_object_open_opts *opts)
>> > +{
>> > + return __bpf_object__open_mem(obj_buf, obj_buf_sz, opts, false);
>> > +}
>> > +
>> > +struct bpf_object *
>> > +bpf_object__open_buffer(const void *obj_buf, size_t obj_buf_sz, const char *name)
>> > +{
>> > + struct bpf_object_open_opts opts = {
>> > + .sz = sizeof(struct bpf_object_open_opts),
>> > + .object_name = name,
>> > + };
>>
>> I think this usage with the "size in struct" model is really awkward.
>> Could we define a macro to help hide it? E.g.,
>>
>> #define BPF_OPTS_TYPE(type) struct bpf_ ## type ## _opts
>> #define DECLARE_BPF_OPTS(var, type) BPF_OPTS_TYPE(type) var = { .sz = sizeof(BPF_OPTS_TYPE(type)); }
>
> We certainly could (though I'd maintain that type specified full
> struct name, makes it easier to navigate/grep code), but then we'll be
> preventing this nice syntax of initializing structs, which makes me
> very said because I love that syntax.
>>
>> Then the usage code could be:
>>
>> DECLARE_BPF_OPTS(opts, object_open);
>> opts.object_name = name;
>>
>> Still not ideal, but at least it's less boiler plate for the caller, and
>> people will be less likely to mess up by forgetting to add the size.
>
> What do you think about this?
>
> #define BPF_OPTS(type, name, ...) \
> struct type name = { \
> .sz = sizeof(struct type), \
> __VA_ARGS__ \
> }
>
> struct bla_opts {
> size_t sz;
> int opt1;
> void *opt2;
> const char *opt3;
> };
>
> int main() {
> BPF_OPTS(bla_opts, opts,
> .opt1 = 123,
> .opt2 = NULL,
> .opt3 = "fancy",
> );
>
> /* then also */
> BPF_OPTS(bla_opts, old_school);
> old_school.opt1 = 256;
>
> return opts.opt1;
> }
Sure, LGTM! Should we still keep the bit where it expands _opts in the
struct name as part of the macro, or does that become too obtuse?
> Thanks a lot for a thoughtful feedback, Toke!
You're very welcome! And thanks for working on these API issues!
-Toke
