[PATCH bpf-next v2 0/2] tools: bpftool: support creating outer maps

[PATCH bpf-next v2 0/2] tools: bpftool: support creating outer maps

[Quentin Monnet]

This series makes bpftool able to create outer maps (maps of types
array-of-maps and hash-of-maps). This is done by passing the relevant
inner_map_fd, which we do through a new command-line keyword.

A first patch also cleans up the function related to dumping map elements.

v2:
- v1 was wrongly expected to allow bpftool to dump the content of outer
  maps (already supported). v2 skipped that patch, and instead replaced it
  with a clean-up for the dump_map_elem() function.

Quentin Monnet (2):
  tools: bpftool: clean up function to dump map entry
  tools: bpftool: add "inner_map" to "bpftool map create" outer maps

 .../bpf/bpftool/Documentation/bpftool-map.rst |  10 +-
 tools/bpf/bpftool/bash-completion/bpftool     |  22 ++-
 tools/bpf/bpftool/map.c                       | 149 ++++++++++--------
 3 files changed, 114 insertions(+), 67 deletions(-)

-- 
2.25.1

[PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Quentin Monnet]

The function used to dump a map entry in bpftool is a bit difficult to
follow, as a consequence to earlier refactorings. There is a variable
("num_elems") which does not appear to be necessary, and the error
handling would look cleaner if moved to its own function. Let's clean it
up. No functional change.

v2:
- v1 was erroneously removing the check on fd maps in an attempt to get
  support for outer map dumps. This is already working. Instead, v2
  focuses on cleaning up the dump_map_elem() function, to avoid
  similar confusion in the future.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 49 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index bc0071228f88..c8159cb4fb1e 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
 	jsonw_end_object(json_wtr);
 }
 
-static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
-			      const char *error_msg)
+static void
+print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
+		      const char *error_msg)
 {
 	int msg_size = strlen(error_msg);
 	bool single_line, break_names;
@@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
 	printf("\n");
 }
 
+static void
+print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
+{
+	/* For prog_array maps or arrays of maps, failure to lookup the value
+	 * means there is no entry for that key. Do not print an error message
+	 * in that case.
+	 */
+	if (map_is_map_of_maps(map_info->type) ||
+	    map_is_map_of_progs(map_info->type))
+		return;
+
+	if (json_output) {
+		jsonw_start_object(json_wtr);	/* entry */
+		jsonw_name(json_wtr, "key");
+		print_hex_data_json(key, map_info->key_size);
+		jsonw_name(json_wtr, "value");
+		jsonw_start_object(json_wtr);	/* error */
+		jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
+		jsonw_end_object(json_wtr);	/* error */
+		jsonw_end_object(json_wtr);	/* entry */
+	} else {
+		const char *msg = NULL;
+
+		if (lookup_errno == ENOENT)
+			msg = "<no entry>";
+		else if (lookup_errno == ENOSPC &&
+			 map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
+			msg = "<cannot read>";
+
+		print_entry_error_msg(map_info, key,
+				      msg ? : strerror(lookup_errno));
+	}
+}
+
 static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 			      unsigned char *value)
 {
@@ -713,56 +748,23 @@ static int dump_map_elem(int fd, void *key, void *value,
 			 struct bpf_map_info *map_info, struct btf *btf,
 			 json_writer_t *btf_wtr)
 {
-	int num_elems = 0;
-	int lookup_errno;
-
-	if (!bpf_map_lookup_elem(fd, key, value)) {
-		if (json_output) {
-			print_entry_json(map_info, key, value, btf);
-		} else {
-			if (btf) {
-				struct btf_dumper d = {
-					.btf = btf,
-					.jw = btf_wtr,
-					.is_plain_text = true,
-				};
-
-				do_dump_btf(&d, map_info, key, value);
-			} else {
-				print_entry_plain(map_info, key, value);
-			}
-			num_elems++;
-		}
-		return num_elems;
+	if (bpf_map_lookup_elem(fd, key, value)) {
+		print_entry_error(map_info, key, errno);
+		return -1;
 	}
 
-	/* lookup error handling */
-	lookup_errno = errno;
-
-	if (map_is_map_of_maps(map_info->type) ||
-	    map_is_map_of_progs(map_info->type))
-		return 0;
-
 	if (json_output) {
-		jsonw_start_object(json_wtr);
-		jsonw_name(json_wtr, "key");
-		print_hex_data_json(key, map_info->key_size);
-		jsonw_name(json_wtr, "value");
-		jsonw_start_object(json_wtr);
-		jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
-		jsonw_end_object(json_wtr);
-		jsonw_end_object(json_wtr);
-	} else {
-		const char *msg = NULL;
-
-		if (lookup_errno == ENOENT)
-			msg = "<no entry>";
-		else if (lookup_errno == ENOSPC &&
-			 map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
-			msg = "<cannot read>";
+		print_entry_json(map_info, key, value, btf);
+	} else if (btf) {
+		struct btf_dumper d = {
+			.btf = btf,
+			.jw = btf_wtr,
+			.is_plain_text = true,
+		};
 
-		print_entry_error(map_info, key,
-				  msg ? : strerror(lookup_errno));
+		do_dump_btf(&d, map_info, key, value);
+	} else {
+		print_entry_plain(map_info, key, value);
 	}
 
 	return 0;
@@ -873,7 +875,8 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
 				err = 0;
 			break;
 		}
-		num_elems += dump_map_elem(fd, key, value, info, btf, wtr);
+		if (!dump_map_elem(fd, key, value, info, btf, wtr))
+			num_elems++;
 		prev_key = key;
 	}
 
-- 
2.25.1

[PATCH bpf-next v2 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps

[Quentin Monnet]

There is no support for creating maps of types array-of-map or
hash-of-map in bpftool. This is because the kernel needs an inner_map_fd
to collect metadata on the inner maps to be supported by the new map,
but bpftool does not provide a way to pass this file descriptor.

Add a new optional "inner_map" keyword that can be used to pass a
reference to a map, retrieve a fd to that map, and pass it as the
inner_map_fd.

Add related documentation and bash completion. Note that we can
reference the inner map by its name, meaning we can have several times
the keyword "name" with different meanings (mandatory outer map name,
and possibly a name to use to find the inner_map_fd). The bash
completion will offer it just once, and will not suggest "name" on the
following command:

    # bpftool map create /sys/fs/bpf/my_outer_map type hash_of_maps \
        inner_map name my_inner_map [TAB]

Fixing that specific case seems too convoluted. Completion will work as
expected, however, if the outer map name comes first and the "inner_map
name ..." is passed second.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
 .../bpf/bpftool/Documentation/bpftool-map.rst | 10 +++-
 tools/bpf/bpftool/bash-completion/bpftool     | 22 ++++++++-
 tools/bpf/bpftool/map.c                       | 48 +++++++++++++------
 3 files changed, 62 insertions(+), 18 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 083db6c2fc67..ca9d62d7e0bd 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -23,7 +23,8 @@ 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*] [**inner_map** *MAP*] \
+|		[**dev** *NAME*]
 |	**bpftool** **map dump**       *MAP*
 |	**bpftool** **map update**     *MAP* [**key** *DATA*] [**value** *VALUE*] [*UPDATE_FLAGS*]
 |	**bpftool** **map lookup**     *MAP* [**key** *DATA*]
@@ -67,7 +68,7 @@ DESCRIPTION
 		  maps. On such kernels bpftool will automatically emit this
 		  information as well.
 
-	**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*] [**inner_map** *MAP*] [**dev** *NAME*]
 		  Create a new map with given parameters and pin it to *bpffs*
 		  as *FILE*.
 
@@ -75,6 +76,11 @@ DESCRIPTION
 		  desired flags, e.g. 1024 for **BPF_F_MMAPABLE** (see bpf.h
 		  UAPI header for existing flags).
 
+		  To create maps of type array-of-maps or hash-of-maps, the
+		  **inner_map** keyword must be used to pass an inner map. The
+		  kernel needs it to collect metadata related to the inner maps
+		  that the new map will work with.
+
 		  Keyword **dev** expects a network interface name, and is used
 		  to request hardware offload for the map.
 
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 7b68e3c0a5fb..3f1da30c4da6 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -709,9 +709,26 @@ _bpftool()
                                                    "$cur" ) )
                             return 0
                             ;;
-                        key|value|flags|name|entries)
+                        key|value|flags|entries)
                             return 0
                             ;;
+                        inner_map)
+                            COMPREPLY=( $( compgen -W "$MAP_TYPE" -- "$cur" ) )
+                            return 0
+                            ;;
+                        id)
+                            _bpftool_get_map_ids
+                            ;;
+                        name)
+                            case $pprev in
+                                inner_map)
+                                    _bpftool_get_map_names
+                                    ;;
+                                *)
+                                    return 0
+                                    ;;
+                            esac
+                            ;;
                         *)
                             _bpftool_once_attr 'type'
                             _bpftool_once_attr 'key'
@@ -719,6 +736,9 @@ _bpftool()
                             _bpftool_once_attr 'entries'
                             _bpftool_once_attr 'name'
                             _bpftool_once_attr 'flags'
+                            if _bpftool_search_list 'array_of_maps' 'hash_of_maps'; then
+                                _bpftool_once_attr 'inner_map'
+                            fi
                             _bpftool_once_attr 'dev'
                             return 0
                             ;;
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index c8159cb4fb1e..7370fdd57de3 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -1250,7 +1250,7 @@ static int do_create(int argc, char **argv)
 {
 	struct bpf_create_map_attr attr = { NULL, };
 	const char *pinfile;
-	int err, fd;
+	int err = -1, fd;
 
 	if (!REQ_ARGS(7))
 		return -1;
@@ -1265,13 +1265,13 @@ static int do_create(int argc, char **argv)
 
 			if (attr.map_type) {
 				p_err("map type already specified");
-				return -1;
+				goto exit;
 			}
 
 			attr.map_type = map_type_from_str(*argv);
 			if ((int)attr.map_type < 0) {
 				p_err("unrecognized map type: %s", *argv);
-				return -1;
+				goto exit;
 			}
 			NEXT_ARG();
 		} else if (is_prefix(*argv, "name")) {
@@ -1280,43 +1280,56 @@ static int do_create(int argc, char **argv)
 		} else if (is_prefix(*argv, "key")) {
 			if (parse_u32_arg(&argc, &argv, &attr.key_size,
 					  "key size"))
-				return -1;
+				goto exit;
 		} else if (is_prefix(*argv, "value")) {
 			if (parse_u32_arg(&argc, &argv, &attr.value_size,
 					  "value size"))
-				return -1;
+				goto exit;
 		} else if (is_prefix(*argv, "entries")) {
 			if (parse_u32_arg(&argc, &argv, &attr.max_entries,
 					  "max entries"))
-				return -1;
+				goto exit;
 		} else if (is_prefix(*argv, "flags")) {
 			if (parse_u32_arg(&argc, &argv, &attr.map_flags,
 					  "flags"))
-				return -1;
+				goto exit;
 		} else if (is_prefix(*argv, "dev")) {
 			NEXT_ARG();
 
 			if (attr.map_ifindex) {
 				p_err("offload device already specified");
-				return -1;
+				goto exit;
 			}
 
 			attr.map_ifindex = if_nametoindex(*argv);
 			if (!attr.map_ifindex) {
 				p_err("unrecognized netdevice '%s': %s",
 				      *argv, strerror(errno));
-				return -1;
+				goto exit;
 			}
 			NEXT_ARG();
+		} else if (is_prefix(*argv, "inner_map")) {
+			struct bpf_map_info info = {};
+			__u32 len = sizeof(info);
+			int inner_map_fd;
+
+			NEXT_ARG();
+			if (!REQ_ARGS(2))
+				usage();
+			inner_map_fd = map_parse_fd_and_info(&argc, &argv,
+							     &info, &len);
+			if (inner_map_fd < 0)
+				return -1;
+			attr.inner_map_fd = inner_map_fd;
 		} else {
 			p_err("unknown arg %s", *argv);
-			return -1;
+			goto exit;
 		}
 	}
 
 	if (!attr.name) {
 		p_err("map name not specified");
-		return -1;
+		goto exit;
 	}
 
 	set_max_rlimit();
@@ -1324,17 +1337,22 @@ static int do_create(int argc, char **argv)
 	fd = bpf_create_map_xattr(&attr);
 	if (fd < 0) {
 		p_err("map create failed: %s", strerror(errno));
-		return -1;
+		goto exit;
 	}
 
 	err = do_pin_fd(fd, pinfile);
 	close(fd);
 	if (err)
-		return err;
+		goto exit;
 
 	if (json_output)
 		jsonw_null(json_wtr);
-	return 0;
+
+exit:
+	if (attr.inner_map_fd > 0)
+		close(attr.inner_map_fd);
+
+	return err;
 }
 
 static int do_pop_dequeue(int argc, char **argv)
@@ -1420,7 +1438,7 @@ static int do_help(int argc, char **argv)
 		"Usage: %1$s %2$s { show | list }   [MAP]\n"
 		"       %1$s %2$s create     FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
 		"                                  entries MAX_ENTRIES name NAME [flags FLAGS] \\\n"
-		"                                  [dev NAME]\n"
+		"                                  [inner_map MAP] [dev NAME]\n"
 		"       %1$s %2$s dump       MAP\n"
 		"       %1$s %2$s update     MAP [key DATA] [value VALUE] [UPDATE_FLAGS]\n"
 		"       %1$s %2$s lookup     MAP [key DATA]\n"
-- 
2.25.1

Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Andrii Nakryiko]

On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> The function used to dump a map entry in bpftool is a bit difficult to
> follow, as a consequence to earlier refactorings. There is a variable
> ("num_elems") which does not appear to be necessary, and the error
> handling would look cleaner if moved to its own function. Let's clean it
> up. No functional change.
>
> v2:
> - v1 was erroneously removing the check on fd maps in an attempt to get
>   support for outer map dumps. This is already working. Instead, v2
>   focuses on cleaning up the dump_map_elem() function, to avoid
>   similar confusion in the future.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
>  1 file changed, 52 insertions(+), 49 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index bc0071228f88..c8159cb4fb1e 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>         jsonw_end_object(json_wtr);
>  }
>
> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> -                             const char *error_msg)
> +static void
> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
> +                     const char *error_msg)
>  {
>         int msg_size = strlen(error_msg);
>         bool single_line, break_names;
> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>         printf("\n");
>  }
>
> +static void
> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
> +{
> +       /* For prog_array maps or arrays of maps, failure to lookup the value
> +        * means there is no entry for that key. Do not print an error message
> +        * in that case.
> +        */

this is the case when error is ENOENT, all the other ones should be
treated the same, no?


> +       if (map_is_map_of_maps(map_info->type) ||
> +           map_is_map_of_progs(map_info->type))
> +               return;
> +
> +       if (json_output) {
> +               jsonw_start_object(json_wtr);   /* entry */
> +               jsonw_name(json_wtr, "key");
> +               print_hex_data_json(key, map_info->key_size);
> +               jsonw_name(json_wtr, "value");
> +               jsonw_start_object(json_wtr);   /* error */
> +               jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
> +               jsonw_end_object(json_wtr);     /* error */
> +               jsonw_end_object(json_wtr);     /* entry */
> +       } else {
> +               const char *msg = NULL;
> +
> +               if (lookup_errno == ENOENT)
> +                       msg = "<no entry>";
> +               else if (lookup_errno == ENOSPC &&
> +                        map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
> +                       msg = "<cannot read>";
> +
> +               print_entry_error_msg(map_info, key,
> +                                     msg ? : strerror(lookup_errno));
> +       }
> +}
> +

[...]

Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Quentin Monnet]

On 09/09/2020 04:25, Andrii Nakryiko wrote:
> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> The function used to dump a map entry in bpftool is a bit difficult to
>> follow, as a consequence to earlier refactorings. There is a variable
>> ("num_elems") which does not appear to be necessary, and the error
>> handling would look cleaner if moved to its own function. Let's clean it
>> up. No functional change.
>>
>> v2:
>> - v1 was erroneously removing the check on fd maps in an attempt to get
>>   support for outer map dumps. This is already working. Instead, v2
>>   focuses on cleaning up the dump_map_elem() function, to avoid
>>   similar confusion in the future.
>>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> ---
>>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index bc0071228f88..c8159cb4fb1e 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>>         jsonw_end_object(json_wtr);
>>  }
>>
>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>> -                             const char *error_msg)
>> +static void
>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
>> +                     const char *error_msg)
>>  {
>>         int msg_size = strlen(error_msg);
>>         bool single_line, break_names;
>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>>         printf("\n");
>>  }
>>
>> +static void
>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
>> +{
>> +       /* For prog_array maps or arrays of maps, failure to lookup the value
>> +        * means there is no entry for that key. Do not print an error message
>> +        * in that case.
>> +        */
> 
> this is the case when error is ENOENT, all the other ones should be
> treated the same, no?

Do you mean all map types should be treated the same? If so, I can
remove the check below, as in v1. Or do you mean there is a missing
check on the error value? In which case I can extend this check to
verify we have ENOENT.

>> +       if (map_is_map_of_maps(map_info->type) ||
>> +           map_is_map_of_progs(map_info->type))
>> +               return;
>> +
>> +       if (json_output) {
>> +               jsonw_start_object(json_wtr);   /* entry */
>> +               jsonw_name(json_wtr, "key");
>> +               print_hex_data_json(key, map_info->key_size);
>> +               jsonw_name(json_wtr, "value");
>> +               jsonw_start_object(json_wtr);   /* error */
>> +               jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
>> +               jsonw_end_object(json_wtr);     /* error */
>> +               jsonw_end_object(json_wtr);     /* entry */
>> +       } else {
>> +               const char *msg = NULL;
>> +
>> +               if (lookup_errno == ENOENT)
>> +                       msg = "<no entry>";
>> +               else if (lookup_errno == ENOSPC &&
>> +                        map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
>> +                       msg = "<cannot read>";
>> +
>> +               print_entry_error_msg(map_info, key,
>> +                                     msg ? : strerror(lookup_errno));
>> +       }
>> +}
>> +
> 
> [...]
> 

Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Andrii Nakryiko]

On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On 09/09/2020 04:25, Andrii Nakryiko wrote:
> > On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> The function used to dump a map entry in bpftool is a bit difficult to
> >> follow, as a consequence to earlier refactorings. There is a variable
> >> ("num_elems") which does not appear to be necessary, and the error
> >> handling would look cleaner if moved to its own function. Let's clean it
> >> up. No functional change.
> >>
> >> v2:
> >> - v1 was erroneously removing the check on fd maps in an attempt to get
> >>   support for outer map dumps. This is already working. Instead, v2
> >>   focuses on cleaning up the dump_map_elem() function, to avoid
> >>   similar confusion in the future.
> >>
> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> >> ---
> >>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
> >>  1 file changed, 52 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> >> index bc0071228f88..c8159cb4fb1e 100644
> >> --- a/tools/bpf/bpftool/map.c
> >> +++ b/tools/bpf/bpftool/map.c
> >> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
> >>         jsonw_end_object(json_wtr);
> >>  }
> >>
> >> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> >> -                             const char *error_msg)
> >> +static void
> >> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
> >> +                     const char *error_msg)
> >>  {
> >>         int msg_size = strlen(error_msg);
> >>         bool single_line, break_names;
> >> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> >>         printf("\n");
> >>  }
> >>
> >> +static void
> >> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
> >> +{
> >> +       /* For prog_array maps or arrays of maps, failure to lookup the value
> >> +        * means there is no entry for that key. Do not print an error message
> >> +        * in that case.
> >> +        */
> >
> > this is the case when error is ENOENT, all the other ones should be
> > treated the same, no?
>
> Do you mean all map types should be treated the same? If so, I can
> remove the check below, as in v1. Or do you mean there is a missing
> check on the error value? In which case I can extend this check to
> verify we have ENOENT.

The former, probably. I don't see how map-in-map is different for
lookups and why it needs special handling.

>
> >> +       if (map_is_map_of_maps(map_info->type) ||
> >> +           map_is_map_of_progs(map_info->type))
> >> +               return;
> >> +
> >> +       if (json_output) {
> >> +               jsonw_start_object(json_wtr);   /* entry */
> >> +               jsonw_name(json_wtr, "key");
> >> +               print_hex_data_json(key, map_info->key_size);
> >> +               jsonw_name(json_wtr, "value");
> >> +               jsonw_start_object(json_wtr);   /* error */
> >> +               jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
> >> +               jsonw_end_object(json_wtr);     /* error */
> >> +               jsonw_end_object(json_wtr);     /* entry */
> >> +       } else {
> >> +               const char *msg = NULL;
> >> +
> >> +               if (lookup_errno == ENOENT)
> >> +                       msg = "<no entry>";
> >> +               else if (lookup_errno == ENOSPC &&
> >> +                        map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
> >> +                       msg = "<cannot read>";
> >> +
> >> +               print_entry_error_msg(map_info, key,
> >> +                                     msg ? : strerror(lookup_errno));
> >> +       }
> >> +}
> >> +
> >
> > [...]
> >
>

Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Quentin Monnet]

On 09/09/2020 17:30, Andrii Nakryiko wrote:
> On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> On 09/09/2020 04:25, Andrii Nakryiko wrote:
>>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>
>>>> The function used to dump a map entry in bpftool is a bit difficult to
>>>> follow, as a consequence to earlier refactorings. There is a variable
>>>> ("num_elems") which does not appear to be necessary, and the error
>>>> handling would look cleaner if moved to its own function. Let's clean it
>>>> up. No functional change.
>>>>
>>>> v2:
>>>> - v1 was erroneously removing the check on fd maps in an attempt to get
>>>>   support for outer map dumps. This is already working. Instead, v2
>>>>   focuses on cleaning up the dump_map_elem() function, to avoid
>>>>   similar confusion in the future.
>>>>
>>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>>>> ---
>>>>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>> index bc0071228f88..c8159cb4fb1e 100644
>>>> --- a/tools/bpf/bpftool/map.c
>>>> +++ b/tools/bpf/bpftool/map.c
>>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>>>>         jsonw_end_object(json_wtr);
>>>>  }
>>>>
>>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>>>> -                             const char *error_msg)
>>>> +static void
>>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
>>>> +                     const char *error_msg)
>>>>  {
>>>>         int msg_size = strlen(error_msg);
>>>>         bool single_line, break_names;
>>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>>>>         printf("\n");
>>>>  }
>>>>
>>>> +static void
>>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
>>>> +{
>>>> +       /* For prog_array maps or arrays of maps, failure to lookup the value
>>>> +        * means there is no entry for that key. Do not print an error message
>>>> +        * in that case.
>>>> +        */
>>>
>>> this is the case when error is ENOENT, all the other ones should be
>>> treated the same, no?
>>
>> Do you mean all map types should be treated the same? If so, I can
>> remove the check below, as in v1. Or do you mean there is a missing
>> check on the error value? In which case I can extend this check to
>> verify we have ENOENT.
> 
> The former, probably. I don't see how map-in-map is different for
> lookups and why it needs special handling.

I didn't find a particular reason in the logs. My guess is that they may
be more likely to have "empty" entries than other types, and that it
might be more difficult to spot the existing entries in the middle of a
list of "<no entry>" messages.

But I agree, let's get rid of this special case and have the same output
for all types. I'll respin.

Thanks again,
Quentin

Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Andrii Nakryiko]

On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On 09/09/2020 17:30, Andrii Nakryiko wrote:
> > On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> On 09/09/2020 04:25, Andrii Nakryiko wrote:
> >>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>>>
> >>>> The function used to dump a map entry in bpftool is a bit difficult to
> >>>> follow, as a consequence to earlier refactorings. There is a variable
> >>>> ("num_elems") which does not appear to be necessary, and the error
> >>>> handling would look cleaner if moved to its own function. Let's clean it
> >>>> up. No functional change.
> >>>>
> >>>> v2:
> >>>> - v1 was erroneously removing the check on fd maps in an attempt to get
> >>>>   support for outer map dumps. This is already working. Instead, v2
> >>>>   focuses on cleaning up the dump_map_elem() function, to avoid
> >>>>   similar confusion in the future.
> >>>>
> >>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> >>>> ---
> >>>>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
> >>>>  1 file changed, 52 insertions(+), 49 deletions(-)
> >>>>
> >>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> >>>> index bc0071228f88..c8159cb4fb1e 100644
> >>>> --- a/tools/bpf/bpftool/map.c
> >>>> +++ b/tools/bpf/bpftool/map.c
> >>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
> >>>>         jsonw_end_object(json_wtr);
> >>>>  }
> >>>>
> >>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> >>>> -                             const char *error_msg)
> >>>> +static void
> >>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
> >>>> +                     const char *error_msg)
> >>>>  {
> >>>>         int msg_size = strlen(error_msg);
> >>>>         bool single_line, break_names;
> >>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> >>>>         printf("\n");
> >>>>  }
> >>>>
> >>>> +static void
> >>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
> >>>> +{
> >>>> +       /* For prog_array maps or arrays of maps, failure to lookup the value
> >>>> +        * means there is no entry for that key. Do not print an error message
> >>>> +        * in that case.
> >>>> +        */
> >>>
> >>> this is the case when error is ENOENT, all the other ones should be
> >>> treated the same, no?
> >>
> >> Do you mean all map types should be treated the same? If so, I can
> >> remove the check below, as in v1. Or do you mean there is a missing
> >> check on the error value? In which case I can extend this check to
> >> verify we have ENOENT.
> >
> > The former, probably. I don't see how map-in-map is different for
> > lookups and why it needs special handling.
>
> I didn't find a particular reason in the logs. My guess is that they may
> be more likely to have "empty" entries than other types, and that it
> might be more difficult to spot the existing entries in the middle of a
> list of "<no entry>" messages.
>
> But I agree, let's get rid of this special case and have the same output
> for all types. I'll respin.

Oh, wait, I think what I had in mind is to special case ENOENT for
map-in-map and just skip those. So yeah, sorry, there is still a bit
of a special handling, but **only** for -ENOENT. When I was replying I
forgot bpftool emits "<no entry>" for each -ENOENT by default.

>
> Thanks again,
> Quentin

Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Quentin Monnet]

On 09/09/2020 17:46, Andrii Nakryiko wrote:
> On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> On 09/09/2020 17:30, Andrii Nakryiko wrote:
>>> On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>
>>>> On 09/09/2020 04:25, Andrii Nakryiko wrote:
>>>>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>>>
>>>>>> The function used to dump a map entry in bpftool is a bit difficult to
>>>>>> follow, as a consequence to earlier refactorings. There is a variable
>>>>>> ("num_elems") which does not appear to be necessary, and the error
>>>>>> handling would look cleaner if moved to its own function. Let's clean it
>>>>>> up. No functional change.
>>>>>>
>>>>>> v2:
>>>>>> - v1 was erroneously removing the check on fd maps in an attempt to get
>>>>>>   support for outer map dumps. This is already working. Instead, v2
>>>>>>   focuses on cleaning up the dump_map_elem() function, to avoid
>>>>>>   similar confusion in the future.
>>>>>>
>>>>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>>>>>> ---
>>>>>>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
>>>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>>>> index bc0071228f88..c8159cb4fb1e 100644
>>>>>> --- a/tools/bpf/bpftool/map.c
>>>>>> +++ b/tools/bpf/bpftool/map.c
>>>>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>>>>>>         jsonw_end_object(json_wtr);
>>>>>>  }
>>>>>>
>>>>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>>>>>> -                             const char *error_msg)
>>>>>> +static void
>>>>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
>>>>>> +                     const char *error_msg)
>>>>>>  {
>>>>>>         int msg_size = strlen(error_msg);
>>>>>>         bool single_line, break_names;
>>>>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>>>>>>         printf("\n");
>>>>>>  }
>>>>>>
>>>>>> +static void
>>>>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
>>>>>> +{
>>>>>> +       /* For prog_array maps or arrays of maps, failure to lookup the value
>>>>>> +        * means there is no entry for that key. Do not print an error message
>>>>>> +        * in that case.
>>>>>> +        */
>>>>>
>>>>> this is the case when error is ENOENT, all the other ones should be
>>>>> treated the same, no?
>>>>
>>>> Do you mean all map types should be treated the same? If so, I can
>>>> remove the check below, as in v1. Or do you mean there is a missing
>>>> check on the error value? In which case I can extend this check to
>>>> verify we have ENOENT.
>>>
>>> The former, probably. I don't see how map-in-map is different for
>>> lookups and why it needs special handling.
>>
>> I didn't find a particular reason in the logs. My guess is that they may
>> be more likely to have "empty" entries than other types, and that it
>> might be more difficult to spot the existing entries in the middle of a
>> list of "<no entry>" messages.
>>
>> But I agree, let's get rid of this special case and have the same output
>> for all types. I'll respin.
> 
> Oh, wait, I think what I had in mind is to special case ENOENT for
> map-in-map and just skip those. So yeah, sorry, there is still a bit
> of a special handling, but **only** for -ENOENT. When I was replying I
> forgot bpftool emits "<no entry>" for each -ENOENT by default.

So do you prefer me to extend the check with errno == -ENOENT? Or shall
I remove it entirely and just have the "<no entry>" messages like for
the other map types?

Quentin

Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Andrii Nakryiko]

On Wed, Sep 9, 2020 at 9:59 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On 09/09/2020 17:46, Andrii Nakryiko wrote:
> > On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> On 09/09/2020 17:30, Andrii Nakryiko wrote:
> >>> On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>>>
> >>>> On 09/09/2020 04:25, Andrii Nakryiko wrote:
> >>>>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>>>>>
> >>>>>> The function used to dump a map entry in bpftool is a bit difficult to
> >>>>>> follow, as a consequence to earlier refactorings. There is a variable
> >>>>>> ("num_elems") which does not appear to be necessary, and the error
> >>>>>> handling would look cleaner if moved to its own function. Let's clean it
> >>>>>> up. No functional change.
> >>>>>>
> >>>>>> v2:
> >>>>>> - v1 was erroneously removing the check on fd maps in an attempt to get
> >>>>>>   support for outer map dumps. This is already working. Instead, v2
> >>>>>>   focuses on cleaning up the dump_map_elem() function, to avoid
> >>>>>>   similar confusion in the future.
> >>>>>>
> >>>>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> >>>>>> ---
> >>>>>>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
> >>>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
> >>>>>>
> >>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> >>>>>> index bc0071228f88..c8159cb4fb1e 100644
> >>>>>> --- a/tools/bpf/bpftool/map.c
> >>>>>> +++ b/tools/bpf/bpftool/map.c
> >>>>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
> >>>>>>         jsonw_end_object(json_wtr);
> >>>>>>  }
> >>>>>>
> >>>>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> >>>>>> -                             const char *error_msg)
> >>>>>> +static void
> >>>>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
> >>>>>> +                     const char *error_msg)
> >>>>>>  {
> >>>>>>         int msg_size = strlen(error_msg);
> >>>>>>         bool single_line, break_names;
> >>>>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> >>>>>>         printf("\n");
> >>>>>>  }
> >>>>>>
> >>>>>> +static void
> >>>>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
> >>>>>> +{
> >>>>>> +       /* For prog_array maps or arrays of maps, failure to lookup the value
> >>>>>> +        * means there is no entry for that key. Do not print an error message
> >>>>>> +        * in that case.
> >>>>>> +        */
> >>>>>
> >>>>> this is the case when error is ENOENT, all the other ones should be
> >>>>> treated the same, no?
> >>>>
> >>>> Do you mean all map types should be treated the same? If so, I can
> >>>> remove the check below, as in v1. Or do you mean there is a missing
> >>>> check on the error value? In which case I can extend this check to
> >>>> verify we have ENOENT.
> >>>
> >>> The former, probably. I don't see how map-in-map is different for
> >>> lookups and why it needs special handling.
> >>
> >> I didn't find a particular reason in the logs. My guess is that they may
> >> be more likely to have "empty" entries than other types, and that it
> >> might be more difficult to spot the existing entries in the middle of a
> >> list of "<no entry>" messages.
> >>
> >> But I agree, let's get rid of this special case and have the same output
> >> for all types. I'll respin.
> >
> > Oh, wait, I think what I had in mind is to special case ENOENT for
> > map-in-map and just skip those. So yeah, sorry, there is still a bit
> > of a special handling, but **only** for -ENOENT. When I was replying I
> > forgot bpftool emits "<no entry>" for each -ENOENT by default.
>
> So do you prefer me to extend the check with errno == -ENOENT? Or shall
> I remove it entirely and just have the "<no entry>" messages like for
> the other map types?

Extend with -ENOENT check.

>
> Quentin