Re: [PATCH RFC bpf-next 1/3] libbpf: Add new bpf_object__load2() using new-style opts

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

On Thu, Dec 19, 2019 at 6:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Since we introduced DECLARE_LIBBPF_OPTS and related macros for declaring
> function options, that is now the preferred way to extend APIs. Introduce a
> variant of the bpf_object__load() function that uses this function, and
> deprecate the _xattr variant. Since all the good function names were taken,
> the new function is unimaginatively called bpf_object__load2().
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

I've been thinking about options for load quite a bit lately, and I'm
leaning towards an opinion, that bpf_object__load() shouldn't take any
options, and all the various per-bpf_object options have to be
specified in bpf_object_open_opts and stored, if necessary for
load/attach phase. So I'd rather move target_btf_path and log_level to
open_opts instead.

For cases where we need to tune load behavior/options per individual
BPF program, bpf_object_load_opts don't work either way, and we'll
have to add some getters/setters for bpf_program objects, anyways. So
I think it's overall cleaner to specify everything per-bpf_object at
"creation time" (which is open), and keep load()/attach() option-less.

>  tools/lib/bpf/libbpf.c   |   31 ++++++++++++++++++-------------
>  tools/lib/bpf/libbpf.h   |   13 +++++++++++++
>  tools/lib/bpf/libbpf.map |    1 +
>  3 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index febbaac3daf4..266b725e444b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4844,15 +4844,12 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>         return 0;
>  }
>
> -int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> +int bpf_object__load2(struct bpf_object *obj,
> +                     const struct bpf_object_load_opts *opts)
>  {
> -       struct bpf_object *obj;
>         int err, i;
>
> -       if (!attr)
> -               return -EINVAL;
> -       obj = attr->obj;
> -       if (!obj)
> +       if (!obj || !OPTS_VALID(opts, bpf_object_load_opts))
>                 return -EINVAL;
>
>         if (obj->loaded) {
> @@ -4867,8 +4864,10 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>         err = err ? : bpf_object__sanitize_and_load_btf(obj);
>         err = err ? : bpf_object__sanitize_maps(obj);
>         err = err ? : bpf_object__create_maps(obj);
> -       err = err ? : bpf_object__relocate(obj, attr->target_btf_path);
> -       err = err ? : bpf_object__load_progs(obj, attr->log_level);
> +       err = err ? : bpf_object__relocate(obj,
> +                                          OPTS_GET(opts, target_btf_path, NULL));
> +       err = err ? : bpf_object__load_progs(obj,
> +                                            OPTS_GET(opts, log_level, 0));
>         if (err)
>                 goto out;
>
> @@ -4884,13 +4883,19 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>         return err;
>  }
>
> -int bpf_object__load(struct bpf_object *obj)
> +int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>  {
> -       struct bpf_object_load_attr attr = {
> -               .obj = obj,
> -       };
> +       DECLARE_LIBBPF_OPTS(bpf_object_load_opts, opts,
> +           .log_level = attr->log_level,
> +           .target_btf_path = attr->target_btf_path,
> +       );
> +
> +       return bpf_object__load2(attr->obj, &opts);
> +}
>
> -       return bpf_object__load_xattr(&attr);
> +int bpf_object__load(struct bpf_object *obj)
> +{
> +       return bpf_object__load2(obj, NULL);
>  }
>
>  static int make_parent_dir(const char *path)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index fe592ef48f1b..ce86277d7445 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -132,8 +132,21 @@ struct bpf_object_load_attr {
>         const char *target_btf_path;
>  };
>
> +struct bpf_object_load_opts {
> +       /* size of this struct, for forward/backward compatiblity */
> +       size_t sz;
> +       /* log level on load */
> +       int log_level;
> +       /* BTF path (for CO-RE relocations) */
> +       const char *target_btf_path;
> +};
> +#define bpf_object_load_opts__last_field target_btf_path
> +
>  /* Load/unload object into/from kernel */
>  LIBBPF_API int bpf_object__load(struct bpf_object *obj);
> +LIBBPF_API int bpf_object__load2(struct bpf_object *obj,
> +                                const struct bpf_object_load_opts *opts);
> +/* deprecated, use bpf_object__load2() instead */
>  LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr);
>  LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index e3a471f38a71..d6cb860763d1 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -217,6 +217,7 @@ LIBBPF_0.0.7 {
>                 bpf_object__attach_skeleton;
>                 bpf_object__destroy_skeleton;
>                 bpf_object__detach_skeleton;
> +               bpf_object__load2;
>                 bpf_object__load_skeleton;
>                 bpf_object__open_skeleton;
>                 bpf_program__attach;
>