Re: [PATCH bpf-next v2 05/12] tools: libbpf: expose the prog type guessing from section name logic

[Andrey Ignatov <rdna@fb.com>]

Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:
> libbpf can guess program type based on ELF section names.  As libbpf
> becomes more popular its association between section name strings and
> types becomes more of a standard.  Allow libbpf users to use the same
> logic for matching strings to types, e.g. when the string originates
> from command line.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
>  tools/lib/bpf/libbpf.c | 43 ++++++++++++++++++++++++------------------
>  tools/lib/bpf/libbpf.h |  3 +++
>  2 files changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 38ed3e92e393..30f3e58bd563 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2081,25 +2081,33 @@ static const struct {
>  #undef BPF_S_PROG_SEC
>  #undef BPF_SA_PROG_SEC
>  
> -static int bpf_program__identify_section(struct bpf_program *prog)
> +int libbpf_prog_type_by_string(const char *name, enum bpf_prog_type *prog_type,
> +			       enum bpf_attach_type *expected_attach_type)
>  {
>  	int i;
>  
> -	if (!prog->section_name)
> -		goto err;
> -
> -	for (i = 0; i < ARRAY_SIZE(section_names); i++)
> -		if (strncmp(prog->section_name, section_names[i].sec,
> -			    section_names[i].len) == 0)
> -			return i;
> -
> -err:
> -	pr_warning("failed to guess program type based on section name %s\n",
> -		   prog->section_name);
> +	if (!name)
> +		return -1;

Should it return -EINVAL? It can help in bpf_prog_load_xattr below:

			err = bpf_program__identify_section(prog, &prog_type,
							    &expected_attach_type);
			if (err < 0) {
				...
				return err;
			}


>  
> +	for (i = 0; i < ARRAY_SIZE(section_names); i++) {
> +		if (strncmp(name, section_names[i].sec, section_names[i].len))
> +			continue;
> +		*prog_type = section_names[i].prog_type;
> +		*expected_attach_type = section_names[i].expected_attach_type;
> +		return 0;
> +	}
>  	return -1;

Same here.

>  }
>  
> +static int
> +bpf_program__identify_section(struct bpf_program *prog,
> +			      enum bpf_prog_type *prog_type,
> +			      enum bpf_attach_type *expected_attach_type)
> +{
> +	return libbpf_prog_type_by_string(prog->section_name, prog_type,
> +					  expected_attach_type);
> +}
> +
>  int bpf_map__fd(struct bpf_map *map)
>  {
>  	return map ? map->fd : -EINVAL;
> @@ -2230,7 +2238,6 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
>  	enum bpf_prog_type prog_type;
>  	struct bpf_object *obj;
>  	struct bpf_map *map;
> -	int section_idx;
>  	int err;
>  
>  	if (!attr)
> @@ -2252,14 +2259,14 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
>  		prog->prog_ifindex = attr->ifindex;
>  		expected_attach_type = attr->expected_attach_type;
>  		if (prog_type == BPF_PROG_TYPE_UNSPEC) {
> -			section_idx = bpf_program__identify_section(prog);
> -			if (section_idx < 0) {
> +			err = bpf_program__identify_section(prog, &prog_type,
> +							    &expected_attach_type);
> +			if (err < 0) {
> +				pr_warning("failed to guess program type based on section name %s\n",
> +					   prog->section_name);
>  				bpf_object__close(obj);
>  				return -EINVAL;
>  			}
> -			prog_type = section_names[section_idx].prog_type;
> -			expected_attach_type =
> -				section_names[section_idx].expected_attach_type;
>  		}
>  
>  		bpf_program__set_type(prog, prog_type);
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 564f4be9bae0..617dacfc6704 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -92,6 +92,9 @@ int bpf_object__set_priv(struct bpf_object *obj, void *priv,
>  			 bpf_object_clear_priv_t clear_priv);
>  void *bpf_object__priv(struct bpf_object *prog);
>  
> +int libbpf_prog_type_by_string(const char *name, enum bpf_prog_type *prog_type,

Nit:

I think it should be either:
  int libbpf_prog_type_by_title(const char *title, enum bpf_prog_type *prog_type,

(to be consistent with bpf_program__title())),

or:
  int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,

(to have function name consistent with argument name and with
bpf_program->name).

IMO "name" is better since it's used across API many times and will be
more consistent, when "title" is used just once (IMO
bpf_program__title() should have been called bpf_program__name() to be
consistent with bpf_map__name() and others, not sure if it's fine to
change now).

> +			       enum bpf_attach_type *expected_attach_type);
> +
>  /* Accessors of bpf_program */
>  struct bpf_program;
>  struct bpf_program *bpf_program__next(struct bpf_program *prog,
> -- 
> 2.17.1
> 

-- 
Andrey Ignatov