Re: [PATCH bpf-next 3/3] bpftool: match maps by name

[Quentin Monnet <quentin.monnet@netronome.com>]

2019-12-10 17:06 UTC+0100 ~ Paul Chaignon <paul.chaignon@orange.com>
> This patch implements lookup by name for maps and changes the behavior of
> lookups by tag to be consistent with prog subcommands.  Similarly to
> program subcommands, the show and dump commands will return all maps with
> the given name (or tag), whereas other commands will error out if several
> maps have the same name (resp. tag).
> 
> When a map has BTF info, it is dumped in JSON with available BTF info.
> This patch requires that all matched maps have BTF info before switching
> the output format to JSON.
> 
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
> ---
>  .../bpf/bpftool/Documentation/bpftool-map.rst |  10 +-
>  tools/bpf/bpftool/bash-completion/bpftool     | 131 ++++++-
>  tools/bpf/bpftool/main.h                      |   2 +-
>  tools/bpf/bpftool/map.c                       | 366 +++++++++++++++---
>  4 files changed, 432 insertions(+), 77 deletions(-)
> 

> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 05b5be4a6ef9..21c676a1eeb1 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool

Nice work on the completion, thanks!

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index de61d73b9030..f0e0be08ba21 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c

[...]

> @@ -654,14 +771,42 @@ static int do_show(int argc, char **argv)
>  		build_pinned_obj_table(&map_table, BPF_OBJ_MAP);
>  
>  	if (argc == 2) {
> -		fd = map_parse_fd_and_info(&argc, &argv, &info, &len);
> -		if (fd < 0)
> +		fds = malloc(sizeof(int));
> +		if (!fds) {
> +			p_err("mem alloc failed");
>  			return -1;
> +		}
> +		nb_fds = map_parse_fds(&argc, &argv, fds);
> +		if (nb_fds < 1)
> +			goto err_free;
> +
> +		if (json_output && nb_fds > 1)
> +			jsonw_start_array(json_wtr);	/* root array */
> +		for (i = 0; i < nb_fds; i++) {
> +			err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
> +			if (err) {
> +				p_err("can't get map info: %s",
> +				      strerror(errno));
> +				for (; i < nb_fds; i++)
> +					close(fds[i]);
> +				goto err_free;

Same remarks as on patch 1, we may want to keep listing the maps even if
we get a failure for one of them?

> +			}
>  
> -		if (json_output)
> -			return show_map_close_json(fd, &info);
> -		else
> -			return show_map_close_plain(fd, &info);
> +			if (json_output)
> +				show_map_close_json(fds[i], &info);
> +			else
> +				show_map_close_plain(fds[i], &info);
> +
> +			close(fds[i]);
> +		}
> +		if (json_output && nb_fds > 1)
> +			jsonw_end_array(json_wtr);	/* root array */
> +
> +		return 0;
> +
> +err_free:
> +		free(fds);
> +		return -1;
>  	}
>  
>  	if (argc)

The rest of the code looks good to me, thanks a lot for working on this!
Quentin