bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Andrii Nakryiko <andriin@fb.com>,
	bpf@vger.kernel.org, netdev@vger.kernel.org, ast@fb.com,
	daniel@iogearbox.net, kpsingh@chromium.org
Cc: andrii.nakryiko@gmail.com, kernel-team@fb.com,
	Andrii Nakryiko <andriin@fb.com>
Subject: Re: [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts
Date: Tue, 01 Oct 2019 10:42:11 +0200	[thread overview]
Message-ID: <871rvwx3fg.fsf@toke.dk> (raw)
In-Reply-To: <20190930164239.3697916-1-andriin@fb.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.


  reply	other threads:[~2019-10-01  8:42 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-30 16:42 [RFC][PATCH bpf-next] libbpf: add bpf_object__open_{file,mem} w/ sized opts Andrii Nakryiko
2019-10-01  8:42 ` Toke Høiland-Jørgensen [this message]
2019-10-01 18:56   ` Andrii Nakryiko
2019-10-01 21:49     ` Toke Høiland-Jørgensen
2019-10-01 23:43       ` Andrii Nakryiko
2019-10-02  6:55         ` Toke Høiland-Jørgensen
2019-10-02 16:55           ` Andrii Nakryiko
2019-10-01 16:48 ` Jesper Dangaard Brouer
2019-10-01 18:59   ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871rvwx3fg.fsf@toke.dk \
    --to=toke@redhat.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=kpsingh@chromium.org \
    --cc=netdev@vger.kernel.org \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).