[RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Andrii Nakryiko]

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.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c          | 56 ++++++++++++++++++++++++++-------
 tools/lib/bpf/libbpf.h          | 33 ++++++++++++++++---
 tools/lib/bpf/libbpf.map        |  6 ++++
 tools/lib/bpf/libbpf_internal.h |  6 ++++
 4 files changed, 85 insertions(+), 16 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e0276520171b..bb8f4a6e4e6b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -255,7 +255,7 @@ struct bpf_object {
 	 */
 	struct {
 		int fd;
-		void *obj_buf;
+		const void *obj_buf;
 		size_t obj_buf_sz;
 		Elf *elf;
 		GElf_Ehdr ehdr;
@@ -492,7 +492,7 @@ bpf_object__init_prog_names(struct bpf_object *obj)
 }
 
 static struct bpf_object *bpf_object__new(const char *path,
-					  void *obj_buf,
+					  const void *obj_buf,
 					  size_t obj_buf_sz)
 {
 	struct bpf_object *obj;
@@ -569,7 +569,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 		 * obj_buf should have been validated by
 		 * bpf_object__open_buffer().
 		 */
-		obj->efile.elf = elf_memory(obj->efile.obj_buf,
+		obj->efile.elf = elf_memory((char *)obj->efile.obj_buf,
 					    obj->efile.obj_buf_sz);
 	} else {
 		obj->efile.fd = open(obj->path, O_RDONLY);
@@ -3597,7 +3597,7 @@ static int bpf_object__validate(struct bpf_object *obj, bool needs_kver)
 }
 
 static struct bpf_object *
-__bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz,
+__bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 		   bool needs_kver, int flags)
 {
 	struct bpf_object *obj;
@@ -3655,25 +3655,59 @@ 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_file(const char *path, struct bpf_object_open_opts *opts)
+{
+	if (!OPTS_VALID(opts) || !path)
+		return ERR_PTR(-EINVAL);
+
+	pr_debug("loading %s\n", path);
+
+	return __bpf_object__open(path, NULL, 0, false, 0);
+}
+
+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)
 {
 	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,
+	};
+
+	if (!obj_buf || obj_buf_sz == 0)
+		return NULL;
+
+	return __bpf_object__open_mem(obj_buf, obj_buf_sz, &opts, true);
 }
 
 int bpf_object__unload(struct bpf_object *obj)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index e8f70977d137..987db195c5a0 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -67,14 +67,37 @@ struct bpf_object_open_attr {
 	enum bpf_prog_type prog_type;
 };
 
+struct bpf_object_open_opts {
+	/* size of this struct, for forward and 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;
+	/* program type to use if determination based on program name doesn't
+	 * work */
+	enum bpf_prog_type fallback_prog_type;
+};
+
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *
+bpf_object__open_file(const char *path, struct bpf_object_open_opts *opts);
+LIBBPF_API struct bpf_object *
+bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
+		     struct bpf_object_open_opts *opts);
+
+/* deprecated bpf_object__open variants */
+LIBBPF_API struct bpf_object *
+bpf_object__open_buffer(const void *obj_buf, size_t obj_buf_sz,
+			const char *name);
+LIBBPF_API struct bpf_object *
 bpf_object__open_xattr(struct bpf_object_open_attr *attr);
-struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
-					    int flags);
-LIBBPF_API struct bpf_object *bpf_object__open_buffer(void *obj_buf,
-						      size_t obj_buf_sz,
-						      const char *name);
+struct bpf_object *
+__bpf_object__open_xattr(struct bpf_object_open_attr *attr, int flags);
+
 int bpf_object__section_size(const struct bpf_object *obj, const char *name,
 			     __u32 *size);
 int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index d04c7cb623ed..4d241fd92dd4 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -190,3 +190,9 @@ LIBBPF_0.0.5 {
 	global:
 		bpf_btf_get_next_id;
 } LIBBPF_0.0.4;
+
+LIBBPF_0.0.6 {
+	global:
+		bpf_object__open_file;
+		bpf_object__open_mem;
+} LIBBPF_0.0.5;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 2e83a34f8c79..1cf2cf8d80f3 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -47,6 +47,12 @@ do {				\
 #define pr_info(fmt, ...)	__pr(LIBBPF_INFO, fmt, ##__VA_ARGS__)
 #define pr_debug(fmt, ...)	__pr(LIBBPF_DEBUG, fmt, ##__VA_ARGS__)
 
+#define OPTS_VALID(opts) (!(opts) || (opts)->sz >= sizeof((opts)->sz))
+#define OPTS_HAS(opts, field) \
+	((opts) && opts->sz >= offsetofend(typeof(*(opts)), field))
+#define OPTS_GET(opts, field, fallback_value) \
+	(OPTS_HAS(opts, field) ? (opts)->field : fallback_value)
+
 int libbpf__load_raw_btf(const char *raw_types, size_t types_len,
 			 const char *str_sec, size_t str_len);
 
-- 
2.17.1

Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andriin@fb.com> 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.

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.

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?

>  	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)); }

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.

-Toke

Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Andrii Nakryiko]

On Tue, Oct 1, 2019 at 1:42 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andriin@fb.com> 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.

>
> 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.

>
> 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?

>
> >       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;
}

Thanks a lot for a thoughtful feedback, Toke!

>
> -Toke
>

Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Tue, Oct 1, 2019 at 1:42 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andriin@fb.com> 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

Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Andrii Nakryiko]

On Tue, Oct 1, 2019 at 2:49 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Tue, Oct 1, 2019 at 1:42 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andriin@fb.com> 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 :)

Ah, cool, glad we are in agreement then :)

>
> >> 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 :)).

Yep, that's what I'm intending to do.

>
> >>
> >> >       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?

For me it's a question of code navigation. When I'll have a code

LIBBPF_OPTS(bpf_object_open, <whatever>);

I'll want to jump to the definition of "bpf_object_open" (e.g., w/
cscope)... and will find nothing, because it's actually
bpf_object_open_opts. So I prefer user to spell it out exactly and in
full, this is more maintainable in the long run, IMO.

I'll work on all of that in non-RFC v1 then.

>
> > Thanks a lot for a thoughtful feedback, Toke!
>
> You're very welcome! And thanks for working on these API issues!
>
> -Toke
>

Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

>> 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?
>
> For me it's a question of code navigation. When I'll have a code
>
> LIBBPF_OPTS(bpf_object_open, <whatever>);
>
> I'll want to jump to the definition of "bpf_object_open" (e.g., w/
> cscope)... and will find nothing, because it's actually
> bpf_object_open_opts. So I prefer user to spell it out exactly and in
> full, this is more maintainable in the long run, IMO.

That's a good point; we shouldn't break cscope!

BTW, speaking of cscope, how about having a 'make cscope' target for
libbpf to generate the definition file? :)

-Toke

Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Andrii Nakryiko]

On Tue, Oct 1, 2019 at 11:56 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> >> 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?
> >
> > For me it's a question of code navigation. When I'll have a code
> >
> > LIBBPF_OPTS(bpf_object_open, <whatever>);
> >
> > I'll want to jump to the definition of "bpf_object_open" (e.g., w/
> > cscope)... and will find nothing, because it's actually
> > bpf_object_open_opts. So I prefer user to spell it out exactly and in
> > full, this is more maintainable in the long run, IMO.
>
> That's a good point; we shouldn't break cscope!
>
> BTW, speaking of cscope, how about having a 'make cscope' target for
> libbpf to generate the definition file? :)

I'm all for it, probably both `make cscope` and `make tags`, like
Linux's make has? Feel free to add them, I can also replicate it to
Github's Makefile after that.

>
> -Toke
>

Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Jesper Dangaard Brouer]

On Mon, 30 Sep 2019 09:42:39 -0700
Andrii Nakryiko <andriin@fb.com> wrote:

> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 2e83a34f8c79..1cf2cf8d80f3 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -47,6 +47,12 @@ do {				\
>  #define pr_info(fmt, ...)	__pr(LIBBPF_INFO, fmt, ##__VA_ARGS__)
>  #define pr_debug(fmt, ...)	__pr(LIBBPF_DEBUG, fmt, ##__VA_ARGS__)
>  
> +#define OPTS_VALID(opts) (!(opts) || (opts)->sz >= sizeof((opts)->sz))

Do be aware that C sizeof() will include the padding the compiler does.
Thus, when extending a struct (e.g. in a newer version) the size
(sizeof) might not actually increase (if compiler padding room exist).

> +#define OPTS_HAS(opts, field) \
> +	((opts) && opts->sz >= offsetofend(typeof(*(opts)), field))
> +#define OPTS_GET(opts, field, fallback_value) \
> +	(OPTS_HAS(opts, field) ? (opts)->field : fallback_value)

I do think, that these two "accessor" defines address the padding issue
I described above.

p.s. I appreciate that you are working on this, and generally like the idea.
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts

[Andrii Nakryiko]

On Tue, Oct 1, 2019 at 9:48 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> On Mon, 30 Sep 2019 09:42:39 -0700
> Andrii Nakryiko <andriin@fb.com> wrote:
>
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 2e83a34f8c79..1cf2cf8d80f3 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -47,6 +47,12 @@ do {                               \
> >  #define pr_info(fmt, ...)    __pr(LIBBPF_INFO, fmt, ##__VA_ARGS__)
> >  #define pr_debug(fmt, ...)   __pr(LIBBPF_DEBUG, fmt, ##__VA_ARGS__)
> >
> > +#define OPTS_VALID(opts) (!(opts) || (opts)->sz >= sizeof((opts)->sz))
>
> Do be aware that C sizeof() will include the padding the compiler does.
> Thus, when extending a struct (e.g. in a newer version) the size
> (sizeof) might not actually increase (if compiler padding room exist).

Yes, that's a very good point, thanks for bringing this up!

I was debating whether OPTS_VALID should validate (similar to kernel)
that all the bytes between user's struct size and our
latest-and-greatest struct definition size are set to zero. But this
padding issue just proves it has to be done always.

And it also shows that using struct initialization to zero (or using
macro with struct field initializers) is almost a must to avoid issue
like this. On libbpf side I think we can just provide helpful message
to user, otherwise it might be hair-pulling to figure out what's
wrong.

>
> > +#define OPTS_HAS(opts, field) \
> > +     ((opts) && opts->sz >= offsetofend(typeof(*(opts)), field))
> > +#define OPTS_GET(opts, field, fallback_value) \
> > +     (OPTS_HAS(opts, field) ? (opts)->field : fallback_value)
>
> I do think, that these two "accessor" defines address the padding issue
> I described above.
>
> p.s. I appreciate that you are working on this, and generally like the idea.

Thanks!

> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer