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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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