Re: [PATCH bpf-next 1/2] tools: bpftool: allow all prog/map handles for pinning objects

[Quentin Monnet <quentin@isovalent.com>]

2020-03-12 18:25 UTC+0000 ~ Quentin Monnet <quentin@isovalent.com>
> Documentation and interactive help for bpftool have always explained
> that the regular handles for programs (id|name|tag|pinned) and maps
> (id|name|pinned) can be passed to the utility when attempting to pin
> objects (bpftool prog pin PROG / bpftool map pin MAP).
> 
> THIS IS A LIE!! The tool actually accepts only ids, as the parsing is
> done in do_pin_any() in common.c instead of reusing the parsing
> functions that have long been generic for program and map handles.
> 
> Instead of fixing the doc, fix the code. It is trivial to reuse the
> generic parsing, and to simplify do_pin_any() in the process.
> 
> Do not accept to pin multiple objects at the same time with
> prog_parse_fds() or map_parse_fds() (this would require a more complex
> syntax for passing multiple sysfs paths and validating that they
> correspond to the number of e.g. programs we find for a given name or
> tag).
> 
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/common.c | 39 +++++---------------------------------
>  tools/bpf/bpftool/main.h   |  2 +-
>  tools/bpf/bpftool/map.c    |  2 +-
>  tools/bpf/bpftool/prog.c   |  2 +-
>  4 files changed, 8 insertions(+), 37 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index b75b8ec5469c..92e51a62bd72 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -211,44 +211,15 @@ int do_pin_fd(int fd, const char *name)
>  	return err;
>  }
>  
> -int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
> +int do_pin_any(int argc, char **argv, int (*get_fd)(int *, char ***))
>  {
> -	unsigned int id;
> -	char *endptr;
> -	int err;
>  	int fd;
>  
> -	if (argc < 3) {
> -		p_err("too few arguments, id ID and FILE path is required");
> -		return -1;
> -	} else if (argc > 3) {
> -		p_err("too many arguments");
> -		return -1;
> -	}
> -
> -	if (!is_prefix(*argv, "id")) {
> -		p_err("expected 'id' got %s", *argv);
> -		return -1;
> -	}
> -	NEXT_ARG();
> -
> -	id = strtoul(*argv, &endptr, 0);
> -	if (*endptr) {
> -		p_err("can't parse %s as ID", *argv);
> -		return -1;
> -	}
> -	NEXT_ARG();
> -
> -	fd = get_fd_by_id(id);
> -	if (fd < 0) {
> -		p_err("can't open object by id (%u): %s", id, strerror(errno));
> -		return -1;
> -	}
> -
> -	err = do_pin_fd(fd, *argv);
> +	fd = get_fd(&argc, &argv);
> +	if (fd < 0)
> +		return fd;
>  
> -	close(fd);
> -	return err;
> +	return do_pin_fd(fd, *argv);

Looks like someone trimmed too much and forgot to close his fd. Will
send v2.

>  }
>