Re: [RFC PATCH v10 22/50] bpf tools: Link all bpf objects onto a list

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

Em Wed, Jul 01, 2015 at 02:14:10AM +0000, Wang Nan escreveu:
> To prevent caller from creating additional structures to hold
> pointers of 'struct bpf_object', this patch link all such

Can you rephrase the above sentence? You mean "to allow enumeration of
all bpf_objects, keep them in a list".

> structures onto a list (hidden to user). bpf_object__for_each() is
> introduced to allow users iterate over each objects.
> bpf_object__for_each() is safe even user close the object during
> iteration.

We have a idiom for that, it s called foo__for_each_entry_safe(), see
list.h, tools/perf/util/evlist.h as well usess those same idioms, please
keep that semantic.

I.e. if one isn't modifying the list while traversing it, no need for
that tmp, if one is, the _safe() variant should be used, that requires
the tmp.

We also have list_first_entry(), list_last_entry(), etc, its good to
keep those semantics, helps people used to kernel (and tools/)
programming to feel at home when reading your code :-)

- Arnaldo
 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
>  tools/lib/bpf/libbpf.c | 32 ++++++++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.h |  7 +++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1c210fb..540ac558 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -17,6 +17,7 @@
>  #include <asm/unistd.h>
>  #include <linux/kernel.h>
>  #include <linux/bpf.h>
> +#include <linux/list.h>
>  #include <libelf.h>
>  #include <gelf.h>
>  
> @@ -104,6 +105,8 @@ struct bpf_program {
>  	bpf_program_clear_priv_t clear_priv;
>  };
>  
> +static LIST_HEAD(bpf_objects_list);
> +
>  struct bpf_object {
>  	char license[64];
>  	u32 kern_version;
> @@ -137,6 +140,12 @@ struct bpf_object {
>  		} *reloc;
>  		int nr_reloc;
>  	} efile;
> +	/*
> +	 * All loaded bpf_object is linked in a list, which is
> +	 * hidden to caller. bpf_objects__<func> handlers deal with
> +	 * all objects.
> +	 */
> +	struct list_head list;
>  	char path[];
>  };
>  #define obj_elf_valid(o)	((o)->efile.elf)
> @@ -254,6 +263,9 @@ static struct bpf_object *bpf_object__new(const char *path,
>  	obj->efile.obj_buf_sz = obj_buf_sz;
>  
>  	obj->loaded = false;
> +
> +	INIT_LIST_HEAD(&obj->list);
> +	list_add(&obj->list, &bpf_objects_list);
>  	return obj;
>  }
>  
> @@ -933,6 +945,7 @@ void bpf_object__close(struct bpf_object *obj)
>  	}
>  	zfree(&obj->programs);
>  
> +	list_del(&obj->list);
>  	free(obj);
>  }
>  
> @@ -945,6 +958,25 @@ int bpf_object__get_prog_cnt(struct bpf_object *obj, size_t *pcnt)
>  	return 0;
>  }
>  
> +struct bpf_object *
> +bpf_object__next(struct bpf_object *prev)
> +{
> +	struct bpf_object *next;
> +
> +	if (!prev)
> +		next = list_first_entry(&bpf_objects_list,
> +					struct bpf_object,
> +					list);
> +	else
> +		next = list_next_entry(prev, list);
> +
> +	/* Empty list is noticed here so don't need checking on entry. */
> +	if (&next->list == &bpf_objects_list)
> +		return NULL;
> +
> +	return next;
> +}
> +
>  struct bpf_program *
>  bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
>  {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index a20ae2e..1cd17a2 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -38,6 +38,13 @@ int bpf_object__unload(struct bpf_object *obj);
>  /* Accessors of bpf_object */
>  int bpf_object__get_prog_cnt(struct bpf_object *obj, size_t *pcnt);
>  
> +struct bpf_object *bpf_object__next(struct bpf_object *prev);
> +#define bpf_object__for_each(pos, tmp)			\
> +	for ((pos) = bpf_object__next(NULL),		\
> +		(tmp) = bpf_object__next(pos);		\
> +	     (pos) != NULL;				\
> +	     (pos) = (tmp), (tmp) = bpf_object__next(tmp))
> +
>  /* Accessors of bpf_program. */
>  struct bpf_program;
>  struct bpf_program *bpf_program__next(struct bpf_program *prog,
> -- 
> 1.8.3.4