[bpf-next,v1,3/7] tools: bpftool: create map of maps

From: Alban Crequy <alban@kinvolk.io>

Before this patch, there was no way to fill attr.inner_map_fd, necessary
for array_of_maps or hash_of_maps.

This patch adds keyword 'innermap' to pass the innermap, either as an id
or as a pinned map.

Example of commands:

$ sudo bpftool map create /sys/fs/bpf/innermap type hash \
        key 8 value 8 entries 64 name innermap flags 1
$ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \
        innermap pinned /sys/fs/bpf/innermap key 64 value 4 \
        entries 64 name myoutermap flags 1
$ sudo bpftool map show pinned /sys/fs/bpf/outermap
47: hash_of_maps  name myoutermap  flags 0x1
	key 64B  value 4B  max_entries 64  memlock 12288B

Documentation and bash completion updated as well.

Signed-off-by: Alban Crequy <alban@kinvolk.io>

---

Previous version of this patch was sent while bpf-next was closed.
https://marc.info/?l=linux-kernel&m=155180393501258&w=2

Since then, the following changes were done:
- error management when calling map_parse_fd (review from Jakub)
- fix documentation and bash completion (review from Quentin)
---
 .../bpf/bpftool/Documentation/bpftool-map.rst |  9 ++-
 tools/bpf/bpftool/bash-completion/bpftool     |  9 +++
 tools/bpf/bpftool/map.c                       | 75 +++++++++++++++++--
 3 files changed, 83 insertions(+), 10 deletions(-)

Hi Alban, thanks for those patches!

2019-03-20 18:33 UTC+0100 ~ Alban Crequy <alban.crequy@gmail.com>
> From: Alban Crequy <alban@kinvolk.io>
> 
> Before this patch, there was no way to fill attr.inner_map_fd, necessary
> for array_of_maps or hash_of_maps.
> 
> This patch adds keyword 'innermap' to pass the innermap, either as an id
> or as a pinned map.
> 
> Example of commands:
> 
> $ sudo bpftool map create /sys/fs/bpf/innermap type hash \
>         key 8 value 8 entries 64 name innermap flags 1
> $ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \
>         innermap pinned /sys/fs/bpf/innermap key 64 value 4 \
>         entries 64 name myoutermap flags 1
> $ sudo bpftool map show pinned /sys/fs/bpf/outermap
> 47: hash_of_maps  name myoutermap  flags 0x1
> 	key 64B  value 4B  max_entries 64  memlock 12288B
> 
> Documentation and bash completion updated as well.
> 
> Signed-off-by: Alban Crequy <alban@kinvolk.io>
> 
> ---
> 
> Previous version of this patch was sent while bpf-next was closed.
> https://marc.info/?l=linux-kernel&m=155180393501258&w=2
> 
> Since then, the following changes were done:
> - error management when calling map_parse_fd (review from Jakub)
> - fix documentation and bash completion (review from Quentin)
> ---
>  .../bpf/bpftool/Documentation/bpftool-map.rst |  9 ++-
>  tools/bpf/bpftool/bash-completion/bpftool     |  9 +++
>  tools/bpf/bpftool/map.c                       | 75 +++++++++++++++++--
>  3 files changed, 83 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> index 5c984ffc9f01..b685641bfd74 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
> @@ -23,7 +23,7 @@ MAP COMMANDS
>  
>  |	**bpftool** **map { show | list }**   [*MAP*]
>  |	**bpftool** **map create**     *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* \
> -|		**entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*]
> +|		**entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] [**innermap** MAP]
>  |	**bpftool** **map dump**       *MAP*
>  |	**bpftool** **map update**     *MAP* [**key** *DATA*] [**value** *VALUE*] [*UPDATE_FLAGS*]
>  |	**bpftool** **map lookup**     *MAP* [**key** *DATA*]
> @@ -60,10 +60,15 @@ DESCRIPTION
>  		  Output will start with map ID followed by map type and
>  		  zero or more named attributes (depending on kernel version).
>  
> -	**bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE*  **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*]
> +	**bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE*  **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] [**innermap** MAP]
>  		  Create a new map with given parameters and pin it to *bpffs*
>  		  as *FILE*.
>  
> +		  To create a map of type **array_of_maps** or
> +		  **hash_of_maps**, the additional parameter **innermap** needs
> +		  to reference an existing map with the type and size of the
> +		  maps in the values.
> +
>  	**bpftool map dump**    *MAP*
>  		  Dump all entries in a given *MAP*.
>  
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index b803827d01e8..9e37de8bb227 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -461,6 +461,14 @@ _bpftool()
>                              _sysfs_get_netdevs
>                              return 0
>                              ;;
> +                        innermap)
> +                            COMPREPLY+=( $( compgen -W "id pinned" -- "$cur" ) )
> +                            return 0
> +                            ;;
> +                        id)
> +                            _bpftool_get_map_ids
> +                            return 0
> +                            ;;
>                          *)
>                              _bpftool_once_attr 'type'
>                              _bpftool_once_attr 'key'
> @@ -469,6 +477,7 @@ _bpftool()
>                              _bpftool_once_attr 'name'
>                              _bpftool_once_attr 'flags'
>                              _bpftool_once_attr 'dev'
> +                            _bpftool_once_attr 'innermap'
>                              return 0
>                              ;;
>                      esac
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 18f9bc3aed4f..a576f2a019be 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -1100,6 +1100,7 @@ static int do_create(int argc, char **argv)
>  	struct bpf_create_map_attr attr = { NULL, };
>  	const char *pinfile;
>  	int err, fd;
> +	int inner_map_fd = -1;

Nit: reverse-Christmas tree for declaring the variables.

>  
>  	if (!REQ_ARGS(7))
>  		return -1;
> @@ -1108,48 +1109,75 @@ static int do_create(int argc, char **argv)
>  	while (argc) {
>  		if (is_prefix(*argv, "type")) {
>  			NEXT_ARG();
> -			if (!REQ_ARGS(1))
> +			if (!REQ_ARGS(1)) {
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
> +			}
>  
>  			if (attr.map_type) {
>  				p_err("map type already specified");
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
>  			}
>  
>  			attr.map_type = map_type_from_str(*argv);
>  			if ((int)attr.map_type < 0) {
>  				p_err("unrecognized map type: %s", *argv);
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
>  			}
>  			NEXT_ARG();
>  		} else if (is_prefix(*argv, "name")) {
>  			NEXT_ARG();
> -			if (!REQ_ARGS(1))
> +			if (!REQ_ARGS(1)) {
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
> +			}
>  			attr.name = GET_ARG();
>  		} else if (is_prefix(*argv, "key")) {
>  			if (parse_u32_arg(&argc, &argv, &attr.key_size,
> -					  "key size"))
> +					  "key size")) {
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
> +			}
>  		} else if (is_prefix(*argv, "value")) {
>  			if (parse_u32_arg(&argc, &argv, &attr.value_size,
> -					  "value size"))
> +					  "value size")) {
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
> +			}
>  		} else if (is_prefix(*argv, "entries")) {
>  			if (parse_u32_arg(&argc, &argv, &attr.max_entries,
> -					  "max entries"))
> +					  "max entries")) {
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
> +			}
>  		} else if (is_prefix(*argv, "flags")) {
>  			if (parse_u32_arg(&argc, &argv, &attr.map_flags,
> -					  "flags"))
> +					  "flags")) {
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
> +			}
>  		} else if (is_prefix(*argv, "dev")) {
>  			NEXT_ARG();
> -			if (!REQ_ARGS(1))
> +			if (!REQ_ARGS(1)) {
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
> +			}
>  
>  			if (attr.map_ifindex) {
>  				p_err("offload device already specified");
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
>  			}
>  
> @@ -1157,28 +1185,59 @@ static int do_create(int argc, char **argv)
>  			if (!attr.map_ifindex) {
>  				p_err("unrecognized netdevice '%s': %s",
>  				      *argv, strerror(errno));
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
>  				return -1;
>  			}
>  			NEXT_ARG();
> +		} else if (is_prefix(*argv, "innermap")) {
> +			NEXT_ARG();
> +			if (!REQ_ARGS(1)) {
> +				if (inner_map_fd != -1)
> +					close(inner_map_fd);
> +				return -1;
> +			}
> +
> +			if (inner_map_fd != -1) {
> +				close(inner_map_fd);
> +				p_err("innermap already specified");
> +				return -1;
> +			}
> +
> +			inner_map_fd = map_parse_fd(&argc, &argv);
> +			if (inner_map_fd < 0)
> +				return -1;
>  		} else {
>  			p_err("unknown arg %s", *argv);
> +			if (inner_map_fd != -1)
> +				close(inner_map_fd);
>  			return -1;
>  		}
>  	}
>  
>  	if (!attr.name) {
>  		p_err("map name not specified");
> +		if (inner_map_fd != -1)
> +			close(inner_map_fd);
>  		return -1;
>  	}
>  
>  	set_max_rlimit();
>  
> +	if (inner_map_fd != -1)
> +		attr.inner_map_fd = inner_map_fd;
> +
>  	fd = bpf_create_map_xattr(&attr);
>  	if (fd < 0) {
>  		p_err("map create failed: %s", strerror(errno));
> +		if (inner_map_fd != -1)
> +			close(inner_map_fd);
>  		return -1;

You might want to move all those

	if (inner_map_fd != -1)
		close(inner_map_fd);
	return -1;

at the end of the function under a label, and jump there with a goto for
the 15 or so occurrences of the pattern?

>  	}
>  
> +	if (inner_map_fd != -1)
> +		close(inner_map_fd);
> +
>  	err = do_pin_fd(fd, pinfile);
>  	close(fd);
>  	if (err)
> @@ -1243,7 +1302,7 @@ static int do_help(int argc, char **argv)
>  		"Usage: %s %s { show | list }   [MAP]\n"
>  		"       %s %s create     FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
>  		"                              entries MAX_ENTRIES name NAME [flags FLAGS] \\\n"
> -		"                              [dev NAME]\n"
> +		"                              [dev NAME] [innermap MAP]\n"
>  		"       %s %s dump       MAP\n"
>  		"       %s %s update     MAP [key DATA] [value VALUE] [UPDATE_FLAGS]\n"
>  		"       %s %s lookup     MAP [key DATA]\n"
>