Re: [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem} w/ extensible opts

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sat, Oct 5, 2019 at 6:24 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Oct 04, 2019 at 03:40:35PM -0700, Andrii Nakryiko wrote:
> > 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, size, and zero
> > 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.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ...
> > +/* Helper macro to declare and initialize libbpf options struct
> > + *
> > + * This dance with uninitialized declaration, followed by memset to zero,
> > + * followed by assignment using compound literal syntax is done to preserve
> > + * ability to use a nice struct field initialization syntax and **hopefully**
> > + * have all the padding bytes initialized to zero. It's not guaranteed though,
> > + * when copying literal, that compiler won't copy garbage in literal's padding
> > + * bytes, but that's the best way I've found and it seems to work in practice.
> > + */
> > +#define LIBBPF_OPTS(TYPE, NAME, ...)                                     \
> > +     struct TYPE NAME;                                                   \
> > +     memset(&NAME, 0, sizeof(struct TYPE));                              \
> > +     NAME = (struct TYPE) {                                              \
> > +             .sz = sizeof(struct TYPE),                                  \
> > +             __VA_ARGS__                                                 \
> > +     }
> > +
> > +struct bpf_object_open_opts {
> > +     /* size of this struct, for forward/backward compatiblity */
> > +     size_t sz;
> > +     /* object name override, if provided:
> > +      * - for object open from file, this will override setting object
> > +      *   name from file path's base name;
> > +      * - for object open from memory buffer, this will specify an object
> > +      *   name and will override default "<addr>-<buf-size>" name;
> > +      */
> > +     const char *object_name;
> > +     /* parse map definitions non-strictly, allowing extra attributes/data */
> > +     bool relaxed_maps;
> > +};
> > +#define bpf_object_open_opts__last_field relaxed_maps
>
> LIBBPF_OPTS macro doesn't inspire confidence, but despite the ugliness
> it is strictly better than what libbpf is using internally to interface
> into kernel via similar bpf_attr api.
> So I think it's an improvement.
> Should this macro be used inside libbpf as well?
> May be rename it too to show that it's not libbpf specific?
>
> Anyhow all that can be done in follow up.
>
> Applied. Thanks
>

Thanks!

I think I'll keep LIBBPF_OPTS because it's specific to this xxx_opts
convention, which has .sz field. bpf_attr doesn't have that. But I'll
create a similar macro for internal libbpf usage and will put it into
bpf_internal.h.