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

Andrii Nakryiko [off-list ref] 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