On Thu, Dec 12, 2019 at 3:34 PM Andrey Ignatov <rdna@xxxxxx> wrote:
>
> Introduce a new bpf_prog_attach_xattr function that accepts an
> extendable struct bpf_prog_attach_opts and supports passing a new
> attribute to BPF_PROG_ATTACH command: replace_prog_fd that is fd of
> previously attached cgroup-bpf program to replace if recently introduced
> BPF_F_REPLACE flag is used.
>
> The new function is named to be consistent with other xattr-functions
> (bpf_prog_test_run_xattr, bpf_create_map_xattr, bpf_load_program_xattr).
>
> The struct bpf_prog_attach_opts is supposed to be used with
> DECLARE_LIBBPF_OPTS framework.
>
> The opts argument is used directly in bpf_prog_attach_xattr
> implementation since at the time of adding all fields already exist in
> the kernel. New fields, if added, will need to be used via OPTS_* macros
> from libbpf_internal.h.
>
> Signed-off-by: Andrey Ignatov <rdna@xxxxxx>
> ---
> tools/lib/bpf/bpf.c | 21 +++++++++++++++++----
> tools/lib/bpf/bpf.h | 12 ++++++++++++
> tools/lib/bpf/libbpf.map | 2 ++
> 3 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 98596e15390f..9f4e42abd185 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -466,14 +466,27 @@ int bpf_obj_get(const char *pathname)
>
> int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
> unsigned int flags)
> +{
> + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, opts,
> + .target_fd = target_fd,
> + .prog_fd = prog_fd,
> + .type = type,
> + .flags = flags,
> + );
> +
> + return bpf_prog_attach_xattr(&opts);
> +}
> +
> +int bpf_prog_attach_xattr(const struct bpf_prog_attach_opts *opts)
When we discussed this whole OPTS idea, we agreed that specifying
mandatory arguments as is makes for better usability. All the optional
stuff then is moved into opts (and then extended indefinitely, because
all the newly added stuff has to be optional, at least for some subset
of arguments).
So if we were to follow those unofficial "guidelines",
bpf_prog_attach_xattr would be defined as:
int bpf_prog_attach_xattr(int prog_fd, int target_fd, enum bpf_attach_type type,
const struct bpf_prog_attach_opts *opts);
, where opts has flags and replace_bpf_fd right now.
Naming wise, it's quite departure from xattr approach, so I'd probably
would go with bpf_prog_attach_opts, but I won't insist.
WDYT?
> {
> union bpf_attr attr;
>
> memset(&attr, 0, sizeof(attr));
> - attr.target_fd = target_fd;
> - attr.attach_bpf_fd = prog_fd;
> - attr.attach_type = type;
> - attr.attach_flags = flags;
> + attr.target_fd = opts->target_fd;
> + attr.attach_bpf_fd = opts->prog_fd;
> + attr.attach_type = opts->type;
> + attr.attach_flags = opts->flags;
> + attr.replace_bpf_fd = opts->replace_prog_fd;
>
> return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
> }
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 5cfe6e0a1aef..5b5f9b374074 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -150,8 +150,20 @@ LIBBPF_API int bpf_map_get_next_key(int fd, const void *key, void *next_key);
> LIBBPF_API int bpf_map_freeze(int fd);
> LIBBPF_API int bpf_obj_pin(int fd, const char *pathname);
> LIBBPF_API int bpf_obj_get(const char *pathname);
> +
> +struct bpf_prog_attach_opts {
> + size_t sz; /* size of this struct for forward/backward compatibility */
> + int target_fd;
> + int prog_fd;
> + enum bpf_attach_type type;
> + unsigned int flags;
> + int replace_prog_fd;
> +};
> +#define bpf_prog_attach_opts__last_field replace_prog_fd
> +
> LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
> enum bpf_attach_type type, unsigned int flags);
> +LIBBPF_API int bpf_prog_attach_xattr(const struct bpf_prog_attach_opts *opts);
> LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
> LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
> enum bpf_attach_type type);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 495df575f87f..42b065454031 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -210,4 +210,6 @@ LIBBPF_0.0.6 {
> } LIBBPF_0.0.5;
>
> LIBBPF_0.0.7 {
> + global:
> + bpf_prog_attach_xattr;
> } LIBBPF_0.0.6;
> --
> 2.17.1
>
