Message-ID: <CAEf4Bzb_8AJS=HLUt9QpdRrt4AzW1ME9tFyL-QTqyu=7fC-dGA@mail.gmail.com>
Date:   Fri, 27 Sep 2019 09:06:41 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     KP Singh <kpsingh@...omium.org>
Cc:     open list <linux-kernel@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>,
        Anton Protopopov <a.s.protopopov@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Florent Revest <revest@...omium.org>
Subject: Re: [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr

On Fri, Sep 27, 2019 at 6:11 AM KP Singh <kpsingh@...omium.org> wrote:
>
> From: KP Singh <kpsingh@...gle.com>
>
> Introduce struct bpf_object_open_buffer_attr and an API function,
> bpf_object__open_xattr, as the existing API, bpf_object__open_buffer,
> doesn't provide a way to specify neither the "needs_kver" nor
> the "flags" parameter to the internal call to the
> __bpf_object__open which makes it inconvenient for loading BPF
> objects that do not require a kernel version from a buffer.
>
> The flags attribute in the bpf_object_open_buffer_attr is set
> to MAPS_RELAX_COMPAT when used in bpf_object__open_buffer to
> maintain backward compatibility as this was added to load objects
> with non-compat map definitions in:
>
> commit c034a177d3c8 ("bpf: bpftool, add flag to allow non-compat map
>                       definitions")
>
> and bpf_object__open_buffer was called with this flag enabled (as a
> boolean true value).
>
> The existing "bpf_object__open_xattr" cannot be modified to
> maintain API compatibility.
>
> Reported-by: Anton Protopopov <a.s.protopopov@...il.com>
> Signed-off-by: KP Singh <kpsingh@...gle.com>
> ---
>  tools/lib/bpf/libbpf.c   | 39 ++++++++++++++++++++++++++++-----------
>  tools/lib/bpf/libbpf.h   | 10 ++++++++++
>  tools/lib/bpf/libbpf.map |  5 +++++
>  3 files changed, 43 insertions(+), 11 deletions(-)
>
> This patch is assimilates the feedback from:
>
>   https://lore.kernel.org/bpf/20190815000330.12044-1-a.s.protopopov@gmail.com/
>
> I have added a "Reported-by:" tag, but please feel free to update to
> "Co-developed-by" if it's more appropriate from an attribution perspective.
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2b57d7ea7836..1f1f2e92832b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2752,25 +2752,42 @@ struct bpf_object *bpf_object__open(const char *path)
>         return bpf_object__open_xattr(&attr);
>  }
>
> -struct bpf_object *bpf_object__open_buffer(void *obj_buf,
> -                                          size_t obj_buf_sz,
> -                                          const char *name)
> +struct bpf_object *
> +bpf_object__open_buffer_xattr(struct bpf_object_open_buffer_attr *attr)

I have few concerns w.r.t. adding API in this form and I'm going to
use this specific case to discuss more general problem of API design,
ABI compatibility, and extending APIs with extra optional arguments.

1. In general, I think it would be good for libbpf API usability to
use the following pattern consistently (moving forward):

T1 some_api_function(T2 mandatory_arg1, ..., TN mandatory_arg, struct
something_opts *opts)

So all the mandatory arguments that have to be provides are specified
explicitly as function arguments. That makes it very clear what API
expects to get always.
opts (we use both opts and attrs, but opts seems better because its
optional options :), on the other hand, is stuff that might be
omitted, so if user doesn't care about tuning behavior of API and
wants all-defaults behavior, then providing NULL here should just
work.

So in this case for bpf_object__open_buffer_xattr(), it could look like this:

struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz,
struct bpf_object_open_opts* opts);

2. Now, we need to do something with adding new options without
breaking ABIs. With all the existing extra attributes, when we need to
add new field to that struct, that can break old code that's
dynamically linked to newer versions of libbpf, because their
attr/opts struct is too short for new code, so that could cause
segment violation or can make libbpf read garbage for those newly
added fields. There are basically three ways we can go about this:

a. either provide the size of opts struct as an extra argument to each
API that uses options, so:
struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz,
struct bpf_object_open_opts* opts, size_t opts_sz);

b. make it mandatory that every option struct has to have as a first
field its size, so:

struct bpf_object_open_opts {
        size_t sz;
        /* now we can keep adding attrs */
};

Now, when options are provided, we'll read first sizeof(size_t) bytes,
validate it for sanity and then we'll know which fields are there or
not.

Both options have downside of user needing to do extra initialization,
but it's not too bad in either case. Especially in case b), if user
doesn't care about extra options, then no extra steps are necessary.
In case a, we can pass NULL, 0 at the end, so also not a big deal.

c. Alternatively, we can do symbol versioning similar how xsk.c
started doing it, and handle those options struct size differences
transparently. But that's a lot of extra boilerplate code in libbpf
and I'd like to avoid that, if possible.

3. Now, the last minor complain is about flags field. It's super
generic. Why not have a set of boolean fields in a struct, in this
case to allow to specify strict/compat modes. Given we solve struct
extensibility issue, adding new bool fields is not an issue at all, so
the benefit of flags field are gone. The downside of flags field is
that it's very opaque integer, you have to go and read sources to
understand all the intended use cases and possible flags, which is
certainly not a great user experience.

I'm curious to hear opinions of people regarding these ABI issues and solutions.


>  {
>         char tmp_name[64];
>

[...]

Powered by blists - more mailing lists