Re: [RFC PATCH v10 20/50] bpf tools: Introduce accessors for struct bpf_program

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Jul 01, 2015 at 02:14:08AM +0000, Wang Nan escreveu:
> This patch introduces accessors for user of libbpf to retrieve section
> name and fd of a opened/loaded eBPF program. 'struct bpf_prog_handler'
> is used for that purpose. Accessors of programs section name and file
> descriptor are provided. Set/get private data are also impelmented.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
>  tools/lib/bpf/libbpf.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.h | 25 +++++++++++++++
>  2 files changed, 107 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 33452f8..e4c5f07 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -98,6 +98,10 @@ struct bpf_program {
>  	int nr_reloc;
>  
>  	int fd;
> +
> +	struct bpf_object *obj;
> +	void *priv;
> +	bpf_program_clear_priv_t clear_priv;
>  };
>  
>  struct bpf_object {
> @@ -150,6 +154,12 @@ static void bpf_program__clear(struct bpf_program *prog)
>  	if (!prog)
>  		return;
>  
> +	if (prog->clear_priv)
> +		prog->clear_priv(prog, prog->priv);
> +
> +	prog->priv = NULL;
> +	prog->clear_priv = NULL;
> +
>  	bpf_program__unload(prog);
>  	zfree(&prog->section_name);
>  	zfree(&prog->insns);
> @@ -211,6 +221,7 @@ bpf_program__new(struct bpf_object *obj, void *data, size_t size,
>  	       prog->insns_cnt * sizeof(struct bpf_insn));
>  	prog->idx = idx;
>  	prog->fd = -1;
> +	prog->obj = obj;
>  
>  	return prog;
>  out:
> @@ -924,3 +935,74 @@ void bpf_object__close(struct bpf_object *obj)
>  
>  	free(obj);
>  }
> +
> +struct bpf_program *
> +bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
> +{
> +	size_t idx;
> +
> +	if (!obj->programs)
> +		return NULL;
> +	/* First handler */
> +	if (prev == NULL)
> +		return &obj->programs[0];
> +
> +	if (prev->obj != obj) {
> +		pr_warning("error: program handler doesn't match object\n");
> +		return NULL;
> +	}
> +
> +	idx = (prev - obj->programs) + 1;
> +	if (idx >= obj->nr_programs)
> +		return NULL;
> +	return &obj->programs[idx];
> +}
> +
> +int bpf_program__set_private(struct bpf_program *prog,
> +			     void *priv,
> +			     bpf_program_clear_priv_t clear_priv)
> +{
> +	if (prog->priv && prog->clear_priv)
> +		prog->clear_priv(prog, prog->priv);
> +
> +	prog->priv = priv;
> +	prog->clear_priv = clear_priv;
> +	return 0;
> +}
> +
> +int bpf_program__get_private(struct bpf_program *prog, void **ppriv)
> +{
> +	*ppriv = prog->priv;
> +	return 0;
> +}
> +
> +int bpf_program__get_title(struct bpf_program *prog,
> +			   const char **ptitle, bool dup)
> +{
> +	const char *title;
> +
> +	if (!ptitle)
> +		return -EINVAL;
> +
> +	title = prog->section_name;
> +	if (dup) {
> +		title = strdup(title);
> +		if (!title) {
> +			pr_warning("failed to strdup program title\n");
> +			*ptitle = NULL;
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	*ptitle = title;
> +	return 0;
> +}
> +
> +int bpf_program__get_fd(struct bpf_program *prog, int *pfd)
> +{
> +	if (!pfd)
> +		return -EINVAL;
> +
> +	*pfd = prog->fd;
> +	return 0;

Why not:

	int bpf_program__fd(struct bpf_program *prog)
	{
		return prog->fd;
	}

Or plain ditch completely this function and go ahead and use:

    prog->fd;

I see, you don't export the bpf_program struct definition, but
then why not return it straight away?

Ditto for bpf_program__get_title()

- Arnaldo

> +}
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 3e69600..9e0e102 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -9,6 +9,7 @@
>  #define __BPF_LIBBPF_H
>  
>  #include <stdio.h>
> +#include <stdbool.h>
>  
>  /*
>   * In include/linux/compiler-gcc.h, __printf is defined. However
> @@ -34,6 +35,30 @@ void bpf_object__close(struct bpf_object *object);
>  int bpf_object__load(struct bpf_object *obj);
>  int bpf_object__unload(struct bpf_object *obj);
>  
> +/* Accessors of bpf_program. */
> +struct bpf_program;
> +struct bpf_program *bpf_program__next(struct bpf_program *prog,
> +				      struct bpf_object *obj);
> +
> +#define bpf_object__for_each_program(pos, obj)		\
> +	for ((pos) = bpf_program__next(NULL, (obj));	\
> +	     (pos) != NULL;				\
> +	     (pos) = bpf_program__next((pos), (obj)))
> +
> +typedef void (*bpf_program_clear_priv_t)(struct bpf_program *,
> +					 void *);
> +
> +int bpf_program__set_private(struct bpf_program *prog, void *priv,
> +			     bpf_program_clear_priv_t clear_priv);
> +
> +int bpf_program__get_private(struct bpf_program *prog,
> +			     void **ppriv);
> +
> +int bpf_program__get_title(struct bpf_program *prog,
> +			   const char **ptitle, bool dup);
> +
> +int bpf_program__get_fd(struct bpf_program *prog, int *pfd);
> +
>  /*
>   * We don't need __attribute__((packed)) now since it is
>   * unnecessary for 'bpf_map_def' because they are all aligned.
> -- 
> 1.8.3.4