[PATCH bpf-next] bpftool: Exit early when it's not possible to dump a REUSEPORT_SOCKARRAY map

[PATCH bpf-next] bpftool: Exit early when it's not possible to dump a REUSEPORT_SOCKARRAY map

[Benjamin Poirier]

avoids outputting a series of
	value:
	No space left on device

The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
only return it if the map was created with value_size = 8. There's nothing
bpftool can do about it. Instead of repeating this error for every key in
the map, print the error once, print an explanatory message and exit.

Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---

Note that this will lead to a merge conflict if the patch "bpftool: Fix
errno variable usage" is merged in the bpf branch:

<<<<<<< HEAD
                if (lookup_errno == ENOENT)
=======
                if (errno == ENOENT) {
>>>>>>> bpf-next

The resolution should be trivial:
                if (lookup_errno == ENOENT) {

 tools/bpf/bpftool/map.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index e96903078991..bb648e42923c 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -684,7 +684,6 @@ 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)) {
@@ -702,9 +701,8 @@ static int dump_map_elem(int fd, void *key, void *value,
 			} else {
 				print_entry_plain(map_info, key, value);
 			}
-			num_elems++;
 		}
-		return num_elems;
+		return 1;
 	}
 
 	/* lookup error handling */
@@ -722,11 +720,13 @@ static int dump_map_elem(int fd, void *key, void *value,
 		jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
 		jsonw_end_object(json_wtr);
 	} else {
-		if (errno == ENOENT)
+		if (errno == ENOENT) {
 			print_entry_plain(map_info, key, NULL);
-		else
+		} else {
 			print_entry_error(map_info, key,
 					  strerror(lookup_errno));
+			return -lookup_errno;
+		}
 	}
 
 	return 0;
@@ -787,7 +787,16 @@ static int do_dump(int argc, char **argv)
 				err = 0;
 			break;
 		}
-		num_elems += dump_map_elem(fd, key, value, &info, btf, btf_wtr);
+		err = dump_map_elem(fd, key, value, &info, btf, btf_wtr);
+		/* bpf_fd_reuseport_array_lookup_elem() can only return a
+		 * value if the map's value_size == 8
+		 */
+		if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
+		    info.value_size != 8 && err == -ENOSPC) {
+			p_err("cannot dump REUSEPORT_SOCKARRAY map with value_size != 8");
+			goto exit_free;
+		}
+		num_elems += err;
 		prev_key = key;
 	}
 
-- 
2.21.0

Re: [PATCH bpf-next] bpftool: Exit early when it's not possible to dump a REUSEPORT_SOCKARRAY map

[Quentin Monnet]

2019-04-11 17:27 UTC+0900 ~ Benjamin Poirier <bpoirier@suse.com>
> avoids outputting a series of
> 	value:
> 	No space left on device
> 
> The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
> only return it if the map was created with value_size = 8. There's nothing
> bpftool can do about it. Instead of repeating this error for every key in
> the map, print the error once, print an explanatory message and exit.
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> 
> Note that this will lead to a merge conflict if the patch "bpftool: Fix
> errno variable usage" is merged in the bpf branch:
> 
> <<<<<<< HEAD
>                 if (lookup_errno == ENOENT)
> =======
>                 if (errno == ENOENT) {
>>>>>>>> bpf-next
> 
> The resolution should be trivial:
>                 if (lookup_errno == ENOENT) {
> 
>  tools/bpf/bpftool/map.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index e96903078991..bb648e42923c 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -684,7 +684,6 @@ 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)) {
> @@ -702,9 +701,8 @@ static int dump_map_elem(int fd, void *key, void *value,
>  			} else {
>  				print_entry_plain(map_info, key, value);
>  			}
> -			num_elems++;
>  		}
> -		return num_elems;
> +		return 1;
>  	}
>  
>  	/* lookup error handling */
> @@ -722,11 +720,13 @@ static int dump_map_elem(int fd, void *key, void *value,
>  		jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
>  		jsonw_end_object(json_wtr);
>  	} else {
> -		if (errno == ENOENT)
> +		if (errno == ENOENT) {
>  			print_entry_plain(map_info, key, NULL);
> -		else
> +		} else {
>  			print_entry_error(map_info, key,
>  					  strerror(lookup_errno));
> +			return -lookup_errno;

Hi, thanks! One comment: if we return -lookup_errno here...

> +		}
>  	}
>  
>  	return 0;
> @@ -787,7 +787,16 @@ static int do_dump(int argc, char **argv)
>  				err = 0;
>  			break;
>  		}
> -		num_elems += dump_map_elem(fd, key, value, &info, btf, btf_wtr);
> +		err = dump_map_elem(fd, key, value, &info, btf, btf_wtr);
> +		/* bpf_fd_reuseport_array_lookup_elem() can only return a
> +		 * value if the map's value_size == 8
> +		 */
> +		if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
> +		    info.value_size != 8 && err == -ENOSPC) {
> +			p_err("cannot dump REUSEPORT_SOCKARRAY map with value_size != 8");
> +			goto exit_free;
> +		}
> +		num_elems += err;

... I think we can have a negative err value here (if we're not in the
case of reuseport_sockarray with wrong map size)? In that case, the
function could print something like "Found -1 elements"?

So maybe update num_elems only if err is positive? Although I'd like
even better if we could somehow find a way to move this error handling
along with the rest of it in dump_map_elem(), and avoid making that
function return either a number of elements or an error depending on the
result from the lookup.

Best regards,
Quentin

Re: [PATCH bpf-next] bpftool: Exit early when it's not possible to dump a REUSEPORT_SOCKARRAY map

[Jakub Kicinski]

On Thu, 11 Apr 2019 17:27:00 +0900, Benjamin Poirier wrote:
> avoids outputting a series of
> 	value:
> 	No space left on device
> 
> The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
> only return it if the map was created with value_size = 8. There's nothing
> bpftool can do about it. Instead of repeating this error for every key in
> the map, print the error once, print an explanatory message and exit.
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>

> @@ -787,7 +787,16 @@ static int do_dump(int argc, char **argv)
>  				err = 0;
>  			break;
>  		}
> -		num_elems += dump_map_elem(fd, key, value, &info, btf, btf_wtr);
> +		err = dump_map_elem(fd, key, value, &info, btf, btf_wtr);
> +		/* bpf_fd_reuseport_array_lookup_elem() can only return a
> +		 * value if the map's value_size == 8
> +		 */
> +		if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
> +		    info.value_size != 8 && err == -ENOSPC) {
> +			p_err("cannot dump REUSEPORT_SOCKARRAY map with value_size != 8");
> +			goto exit_free;

Hmm... could it still be useful to walk the map to see the keys, even
if the kernel can't return meaningful values?

> +		}
> +		num_elems += err;
>  		prev_key = key;
>  	}
>  

[PATCH v2 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Benjamin Poirier]

Commit bf598a8f0f77 ("bpftool: Improve handling of ENOENT on map dumps")
used print_entry_plain() in case of ENOENT because that function provided
the desired formatting. However, that commit actually introduces dead code.
Per-cpu maps are zero-filled. When reading them, it's all or nothing. There
will never be a case where some cpus have an entry and others don't.

The truth is that ENOENT is an error case. So rework print_entry_error() to
provide the desired formatting.

Note that this commit changes the output format in case of errors other
than ENOENT.

example before:
key:
00 00 00 00
value:
No space left on device
[...]

example after:
key: 00 00 00 00
value:
No space left on device
[...]

The ENOENT case is unchanged:
key: 00 00 00 00  value: 14 5b 00 00 00 00 00 00
key: 01 00 00 00  value: <no entry>
[...]

Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
 tools/bpf/bpftool/map.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index e96903078991..71840faaeab5 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -261,20 +261,19 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
 }
 
 static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
-			      const char *value)
+			      const char *value, bool single_line)
 {
-	int value_size = strlen(value);
-	bool single_line, break_names;
+	bool break_names;
 
-	break_names = info->key_size > 16 || value_size > 16;
-	single_line = info->key_size + value_size <= 24 && !break_names;
+	break_names = info->key_size > 16;
+	single_line = single_line && !break_names;
 
 	printf("key:%c", break_names ? '\n' : ' ');
 	fprint_hex(stdout, key, info->key_size, " ");
 
 	printf(single_line ? "  " : "\n");
 
-	printf("value:%c%s", break_names ? '\n' : ' ', value);
+	printf("value:%c%s", single_line ? ' ' : '\n', value);
 
 	printf("\n");
 }
@@ -298,11 +297,7 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 
 		if (info->value_size) {
 			printf("value:%c", break_names ? '\n' : ' ');
-			if (value)
-				fprint_hex(stdout, value, info->value_size,
-					   " ");
-			else
-				printf("<no entry>");
+			fprint_hex(stdout, value, info->value_size, " ");
 		}
 
 		printf("\n");
@@ -321,11 +316,8 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 			for (i = 0; i < n; i++) {
 				printf("value (CPU %02d):%c",
 				       i, info->value_size > 16 ? '\n' : ' ');
-				if (value)
-					fprint_hex(stdout, value + i * step,
-						   info->value_size, " ");
-				else
-					printf("<no entry>");
+				fprint_hex(stdout, value + i * step,
+					   info->value_size, " ");
 				printf("\n");
 			}
 		}
@@ -722,11 +714,13 @@ static int dump_map_elem(int fd, void *key, void *value,
 		jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
 		jsonw_end_object(json_wtr);
 	} else {
+		const char *msg = NULL;
+
 		if (errno == ENOENT)
-			print_entry_plain(map_info, key, NULL);
-		else
-			print_entry_error(map_info, key,
-					  strerror(lookup_errno));
+			msg = "<no entry>";
+
+		print_entry_error(map_info, key,
+				  msg ? : strerror(lookup_errno), !!msg);
 	}
 
 	return 0;
-- 
2.21.0

[PATCH v2 bpf-next 2/2] bpftool: Improve handling of ENOSPC on reuseport_array map dumps

[Benjamin Poirier]

avoids outputting a series of
	value:
	No space left on device

The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
only return it if the map was created with value_size = 8. There's nothing
bpftool can do about it. Instead of repeating this error for every key in
the map, print an explanatory warning and a specialized error.

example before:
key: 00 00 00 00
value:
No space left on device
key: 01 00 00 00
value:
No space left on device
key: 02 00 00 00
value:
No space left on device
Found 0 elements

example after:
Warning: cannot read values from reuseport_sockarray map with value_size != 8
key: 00 00 00 00  value: <cannot read>
key: 01 00 00 00  value: <cannot read>
key: 02 00 00 00  value: <cannot read>
Found 0 elements

Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
 tools/bpf/bpftool/map.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 71840faaeab5..e6d72f777767 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -716,8 +716,12 @@ static int dump_map_elem(int fd, void *key, void *value,
 	} else {
 		const char *msg = NULL;
 
-		if (errno == ENOENT)
+		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(map_info, key,
 				  msg ? : strerror(lookup_errno), !!msg);
@@ -774,6 +778,11 @@ static int do_dump(int argc, char **argv)
 			}
 		}
 
+	if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
+	    info.value_size != 8) {
+		p_info("Warning: cannot read values from %s map with value_size != 8",
+		       map_type_name[info.type]);
+	}
 	while (true) {
 		err = bpf_map_get_next_key(fd, prev_key, key);
 		if (err) {
-- 
2.21.0

Re: [PATCH v2 bpf-next 2/2] bpftool: Improve handling of ENOSPC on reuseport_array map dumps

[Quentin Monnet]

2019-04-12 12:03 UTC+0900 ~ Benjamin Poirier <bpoirier@suse.com>
> avoids outputting a series of
> 	value:
> 	No space left on device
> 
> The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
> only return it if the map was created with value_size = 8. There's nothing
> bpftool can do about it. Instead of repeating this error for every key in
> the map, print an explanatory warning and a specialized error.
> 
> example before:
> key: 00 00 00 00
> value:
> No space left on device
> key: 01 00 00 00
> value:
> No space left on device
> key: 02 00 00 00
> value:
> No space left on device
> Found 0 elements
> 
> example after:
> Warning: cannot read values from reuseport_sockarray map with value_size != 8
> key: 00 00 00 00  value: <cannot read>
> key: 01 00 00 00  value: <cannot read>
> key: 02 00 00 00  value: <cannot read>
> Found 0 elements
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
>   tools/bpf/bpftool/map.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 71840faaeab5..e6d72f777767 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -716,8 +716,12 @@ static int dump_map_elem(int fd, void *key, void *value,
>   	} else {
>   		const char *msg = NULL;
>   
> -		if (errno == ENOENT)
> +		if (lookup_errno == ENOENT) {
>   			msg = "<no entry>";
> +		} else if (lookup_errno == ENOSPC && map_info->type ==
> +			 BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) {

Nit: This should be aligned on the open parenthesis (And preferably 
split after the "&&", if the second condition fits on its own line?).

Thanks for the changes! This version looks much better to me than v1.

Quentin

Re: [PATCH v2 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Quentin Monnet]

2019-04-12 12:03 UTC+0900 ~ Benjamin Poirier <bpoirier@suse.com>
> Commit bf598a8f0f77 ("bpftool: Improve handling of ENOENT on map dumps")
> used print_entry_plain() in case of ENOENT because that function provided
> the desired formatting. However, that commit actually introduces dead code.
> Per-cpu maps are zero-filled. When reading them, it's all or nothing. There
> will never be a case where some cpus have an entry and others don't.
> 
> The truth is that ENOENT is an error case. So rework print_entry_error() to
> provide the desired formatting.
> 
> Note that this commit changes the output format in case of errors other
> than ENOENT.
> 
> example before:
> key:
> 00 00 00 00
> value:
> No space left on device
> [...]
> 
> example after:
> key: 00 00 00 00
> value:
> No space left on device
> [...]
> 
> The ENOENT case is unchanged:
> key: 00 00 00 00  value: 14 5b 00 00 00 00 00 00
> key: 01 00 00 00  value: <no entry>
> [...]
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
>   tools/bpf/bpftool/map.c | 34 ++++++++++++++--------------------
>   1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index e96903078991..71840faaeab5 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -261,20 +261,19 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>   }
>   
>   static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> -			      const char *value)
> +			      const char *value, bool single_line)

Nit: if you respin the series, could you rename "value" into "error_msg" 
or something like this to better indicate we never pass an actual map 
value to the function?

>   {
> -	int value_size = strlen(value);
> -	bool single_line, break_names;
> +	bool break_names;
>   
> -	break_names = info->key_size > 16 || value_size > 16;
> -	single_line = info->key_size + value_size <= 24 && !break_names;
> +	break_names = info->key_size > 16;
> +	single_line = single_line && !break_names;

If I understand correctly, this will also change formatting when error 
message is short (shorter than 16 characters: the "value" line will now 
be unconditionally split, even for short error messages (other than "<no 
entry>")). Why removing the condition on value_size > 16? (This is just 
a remark, I am not against changing it.)

>   
>   	printf("key:%c", break_names ? '\n' : ' ');
>   	fprint_hex(stdout, key, info->key_size, " ");
>   
>   	printf(single_line ? "  " : "\n");
>   
> -	printf("value:%c%s", break_names ? '\n' : ' ', value);
> +	printf("value:%c%s", single_line ? ' ' : '\n', value);
>   
>   	printf("\n");
>   }

[...]

Re: [PATCH v2 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Benjamin Poirier]

On 2019/04/12 11:28, Quentin Monnet wrote:
[...]
> 
> >   {
> > -	int value_size = strlen(value);
> > -	bool single_line, break_names;
> > +	bool break_names;
> > -	break_names = info->key_size > 16 || value_size > 16;
> > -	single_line = info->key_size + value_size <= 24 && !break_names;
> > +	break_names = info->key_size > 16;
> > +	single_line = single_line && !break_names;
> 
> If I understand correctly, this will also change formatting when error
> message is short (shorter than 16 characters: the "value" line will now be
> unconditionally split, even for short error messages (other than "<no
> entry>")). Why removing the condition on value_size > 16? (This is just a
> remark, I am not against changing it.)
> 

With this patch, the error messages from bpftool ("<no entry>", "<cannot
read>"), which are chosen to be short, appear on the same line and the
messages from strerror appear on a separate line. Because those latter
messages are from a source external to bpftool and their length is
unknown ahead of time, I felt it led to a more predictable output to
consistently put them on their own line.

To be honest, I don't think the formatting in those print_entry_*
functions should change according to the length in any case. I think the
key and value for each entry should always be on the same line for ease
of grepping. A followup patch maybe...

Re: [PATCH v2 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Jakub Kicinski]

On Sat, 13 Apr 2019 07:49:24 +0900, Benjamin Poirier wrote:
> To be honest, I don't think the formatting in those print_entry_*
> functions should change according to the length in any case. I think the
> key and value for each entry should always be on the same line for ease
> of grepping. A followup patch maybe...

No, no, please never grep bpftool output.  We have JSON output,
please use JQ: http://manpages.ubuntu.com/manpages/bionic/man1/jq.1.html

Re: [PATCH v2 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Jakub Kicinski]

On Fri, 12 Apr 2019 12:03:21 +0900, Benjamin Poirier wrote:
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index e96903078991..71840faaeab5 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -261,20 +261,19 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>  }
>  
>  static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> -			      const char *value)
> +			      const char *value, bool single_line)
>  {
> -	int value_size = strlen(value);
> -	bool single_line, break_names;
> +	bool break_names;

I'd rather you kept the strlen() and 16 char limitation logic.

> -	break_names = info->key_size > 16 || value_size > 16;
> -	single_line = info->key_size + value_size <= 24 && !break_names;
> +	break_names = info->key_size > 16;
> +	single_line = single_line && !break_names;
>  
>  	printf("key:%c", break_names ? '\n' : ' ');
>  	fprint_hex(stdout, key, info->key_size, " ");
>  
>  	printf(single_line ? "  " : "\n");
>  
> -	printf("value:%c%s", break_names ? '\n' : ' ', value);
> +	printf("value:%c%s", single_line ? ' ' : '\n', value);
>  
>  	printf("\n");
>  }

Re: [PATCH v2 bpf-next 2/2] bpftool: Improve handling of ENOSPC on reuseport_array map dumps

[Jakub Kicinski]

On Fri, 12 Apr 2019 12:03:22 +0900, Benjamin Poirier wrote:
> avoids outputting a series of
> 	value:
> 	No space left on device
> 
> The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
> only return it if the map was created with value_size = 8. There's nothing
> bpftool can do about it. Instead of repeating this error for every key in
> the map, print an explanatory warning and a specialized error.
> 
> example before:
> key: 00 00 00 00
> value:
> No space left on device
> key: 01 00 00 00
> value:
> No space left on device
> key: 02 00 00 00
> value:
> No space left on device
> Found 0 elements
> 
> example after:
> Warning: cannot read values from reuseport_sockarray map with value_size != 8
> key: 00 00 00 00  value: <cannot read>
> key: 01 00 00 00  value: <cannot read>
> key: 02 00 00 00  value: <cannot read>
> Found 0 elements
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
>  tools/bpf/bpftool/map.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 71840faaeab5..e6d72f777767 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -716,8 +716,12 @@ static int dump_map_elem(int fd, void *key, void *value,
>  	} else {
>  		const char *msg = NULL;
>  
> -		if (errno == ENOENT)
> +		if (lookup_errno == ENOENT) {
>  			msg = "<no entry>";
> +		} else if (lookup_errno == ENOSPC && map_info->type ==
> +			 BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) {
> +			msg = "<cannot read>";
> +		}

In addition to Quentin's nit please don't use the curly brackets for
single-line statements.

>  		print_entry_error(map_info, key,
>  				  msg ? : strerror(lookup_errno), !!msg);
> @@ -774,6 +778,11 @@ static int do_dump(int argc, char **argv)
>  			}
>  		}
>  
> +	if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
> +	    info.value_size != 8) {
> +		p_info("Warning: cannot read values from %s map with value_size != 8",
> +		       map_type_name[info.type]);
> +	}

Same here :)

>  	while (true) {
>  		err = bpf_map_get_next_key(fd, prev_key, key);
>  		if (err) {

Re: [PATCH v2 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Benjamin Poirier]

On 2019/04/12 16:53, Jakub Kicinski wrote:
> On Sat, 13 Apr 2019 07:49:24 +0900, Benjamin Poirier wrote:
> > To be honest, I don't think the formatting in those print_entry_*
> > functions should change according to the length in any case. I think the
> > key and value for each entry should always be on the same line for ease
> > of grepping. A followup patch maybe...
> 
> No, no, please never grep bpftool output.  We have JSON output,
> please use JQ: http://manpages.ubuntu.com/manpages/bionic/man1/jq.1.html
> 

Fair enough. I didn't know about jq, thanks.

[PATCH v3 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Benjamin Poirier]

Commit bf598a8f0f77 ("bpftool: Improve handling of ENOENT on map dumps")
used print_entry_plain() in case of ENOENT. However, that commit introduces
dead code. Per-cpu maps are zero-filled. When reading them, it's all or
nothing. There will never be a case where some cpus have an entry and
others don't.

The truth is that ENOENT is an error case. Use print_entry_error() to
output the desired message. That function's "value" parameter is also
renamed to indicate that we never use it for an actual map value.

The output format is unchanged.

Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
 tools/bpf/bpftool/map.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index e96903078991..df958af56b6c 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -261,20 +261,20 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
 }
 
 static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
-			      const char *value)
+			      const char *error_msg)
 {
-	int value_size = strlen(value);
+	int msg_size = strlen(error_msg);
 	bool single_line, break_names;
 
-	break_names = info->key_size > 16 || value_size > 16;
-	single_line = info->key_size + value_size <= 24 && !break_names;
+	break_names = info->key_size > 16 || msg_size > 16;
+	single_line = info->key_size + msg_size <= 24 && !break_names;
 
 	printf("key:%c", break_names ? '\n' : ' ');
 	fprint_hex(stdout, key, info->key_size, " ");
 
 	printf(single_line ? "  " : "\n");
 
-	printf("value:%c%s", break_names ? '\n' : ' ', value);
+	printf("value:%c%s", break_names ? '\n' : ' ', error_msg);
 
 	printf("\n");
 }
@@ -298,11 +298,7 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 
 		if (info->value_size) {
 			printf("value:%c", break_names ? '\n' : ' ');
-			if (value)
-				fprint_hex(stdout, value, info->value_size,
-					   " ");
-			else
-				printf("<no entry>");
+			fprint_hex(stdout, value, info->value_size, " ");
 		}
 
 		printf("\n");
@@ -321,11 +317,8 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 			for (i = 0; i < n; i++) {
 				printf("value (CPU %02d):%c",
 				       i, info->value_size > 16 ? '\n' : ' ');
-				if (value)
-					fprint_hex(stdout, value + i * step,
-						   info->value_size, " ");
-				else
-					printf("<no entry>");
+				fprint_hex(stdout, value + i * step,
+					   info->value_size, " ");
 				printf("\n");
 			}
 		}
@@ -722,11 +715,13 @@ static int dump_map_elem(int fd, void *key, void *value,
 		jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
 		jsonw_end_object(json_wtr);
 	} else {
+		const char *msg = NULL;
+
 		if (errno == ENOENT)
-			print_entry_plain(map_info, key, NULL);
-		else
-			print_entry_error(map_info, key,
-					  strerror(lookup_errno));
+			msg = "<no entry>";
+
+		print_entry_error(map_info, key,
+				  msg ? : strerror(lookup_errno));
 	}
 
 	return 0;
-- 
2.21.0

[PATCH v3 bpf-next 2/2] bpftool: Improve handling of ENOSPC on reuseport_array map dumps

[Benjamin Poirier]

avoids outputting a series of
	value:
	No space left on device

The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
only return it if the map was created with value_size = 8. There's nothing
bpftool can do about it. Instead of repeating this error for every key in
the map, print an explanatory warning and a specialized error.

example before:
key: 00 00 00 00
value:
No space left on device
key: 01 00 00 00
value:
No space left on device
key: 02 00 00 00
value:
No space left on device
Found 0 elements

example after:
Warning: cannot read values from reuseport_sockarray map with value_size != 8
key: 00 00 00 00  value: <cannot read>
key: 01 00 00 00  value: <cannot read>
key: 02 00 00 00  value: <cannot read>
Found 0 elements

Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
 tools/bpf/bpftool/map.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index df958af56b6c..44b192e87708 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -719,6 +719,9 @@ static int dump_map_elem(int fd, void *key, void *value,
 
 		if (errno == ENOENT)
 			msg = "<no entry>";
+		else if (lookup_errno == ENOSPC &&
+			 map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
+			msg = "<cannot read>";
 
 		print_entry_error(map_info, key,
 				  msg ? : strerror(lookup_errno));
@@ -775,6 +778,10 @@ static int do_dump(int argc, char **argv)
 			}
 		}
 
+	if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
+	    info.value_size != 8)
+		p_info("Warning: cannot read values from %s map with value_size != 8",
+		       map_type_name[info.type]);
 	while (true) {
 		err = bpf_map_get_next_key(fd, prev_key, key);
 		if (err) {
-- 
2.21.0

Re: [PATCH v3 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Quentin Monnet]

2019-04-15 16:15 UTC+0900 ~ Benjamin Poirier <bpoirier@suse.com>
> Commit bf598a8f0f77 ("bpftool: Improve handling of ENOENT on map dumps")
> used print_entry_plain() in case of ENOENT. However, that commit introduces
> dead code. Per-cpu maps are zero-filled. When reading them, it's all or
> nothing. There will never be a case where some cpus have an entry and
> others don't.
> 
> The truth is that ENOENT is an error case. Use print_entry_error() to
> output the desired message. That function's "value" parameter is also
> renamed to indicate that we never use it for an actual map value.
> 
> The output format is unchanged.
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
>   tools/bpf/bpftool/map.c | 33 ++++++++++++++-------------------
>   1 file changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index e96903078991..df958af56b6c 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -261,20 +261,20 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>   }
>   
>   static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> -			      const char *value)
> +			      const char *error_msg)
>   {
> -	int value_size = strlen(value);
> +	int msg_size = strlen(error_msg);
>   	bool single_line, break_names;
>   
> -	break_names = info->key_size > 16 || value_size > 16;
> -	single_line = info->key_size + value_size <= 24 && !break_names;
> +	break_names = info->key_size > 16 || msg_size > 16;
> +	single_line = info->key_size + msg_size <= 24 && !break_names;
>   
>   	printf("key:%c", break_names ? '\n' : ' ');
>   	fprint_hex(stdout, key, info->key_size, " ");
>   
>   	printf(single_line ? "  " : "\n");
>   
> -	printf("value:%c%s", break_names ? '\n' : ' ', value);
> +	printf("value:%c%s", break_names ? '\n' : ' ', error_msg);
>   
>   	printf("\n");
>   }
> @@ -298,11 +298,7 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>   
>   		if (info->value_size) {
>   			printf("value:%c", break_names ? '\n' : ' ');
> -			if (value)
> -				fprint_hex(stdout, value, info->value_size,
> -					   " ");
> -			else
> -				printf("<no entry>");
> +			fprint_hex(stdout, value, info->value_size, " ");
>   		}
>   
>   		printf("\n");
> @@ -321,11 +317,8 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>   			for (i = 0; i < n; i++) {
>   				printf("value (CPU %02d):%c",
>   				       i, info->value_size > 16 ? '\n' : ' ');
> -				if (value)
> -					fprint_hex(stdout, value + i * step,
> -						   info->value_size, " ");
> -				else
> -					printf("<no entry>");
> +				fprint_hex(stdout, value + i * step,
> +					   info->value_size, " ");
>   				printf("\n");
>   			}
>   		}
> @@ -722,11 +715,13 @@ static int dump_map_elem(int fd, void *key, void *value,
>   		jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
>   		jsonw_end_object(json_wtr);
>   	} else {
> +		const char *msg = NULL;
> +
>   		if (errno == ENOENT)
> -			print_entry_plain(map_info, key, NULL);
> -		else
> -			print_entry_error(map_info, key,
> -					  strerror(lookup_errno));
> +			msg = "<no entry>";
> +
> +		print_entry_error(map_info, key,
> +				  msg ? : strerror(lookup_errno));

(Nit: This would now fit on a single line, but no need to respin just 
for that.)

Thanks!
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>

Re: [PATCH v3 bpf-next 2/2] bpftool: Improve handling of ENOSPC on reuseport_array map dumps

[Quentin Monnet]

2019-04-15 16:15 UTC+0900 ~ Benjamin Poirier <bpoirier@suse.com>
> avoids outputting a series of
> 	value:
> 	No space left on device
> 
> The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
> only return it if the map was created with value_size = 8. There's nothing
> bpftool can do about it. Instead of repeating this error for every key in
> the map, print an explanatory warning and a specialized error.
> 
> example before:
> key: 00 00 00 00
> value:
> No space left on device
> key: 01 00 00 00
> value:
> No space left on device
> key: 02 00 00 00
> value:
> No space left on device
> Found 0 elements
> 
> example after:
> Warning: cannot read values from reuseport_sockarray map with value_size != 8
> key: 00 00 00 00  value: <cannot read>
> key: 01 00 00 00  value: <cannot read>
> key: 02 00 00 00  value: <cannot read>
> Found 0 elements
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
>   tools/bpf/bpftool/map.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index df958af56b6c..44b192e87708 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -719,6 +719,9 @@ static int dump_map_elem(int fd, void *key, void *value,
>   
>   		if (errno == ENOENT)
>   			msg = "<no entry>";
> +		else if (lookup_errno == ENOSPC &&
> +			 map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
> +			msg = "<cannot read>";
>   
>   		print_entry_error(map_info, key,
>   				  msg ? : strerror(lookup_errno));
> @@ -775,6 +778,10 @@ static int do_dump(int argc, char **argv)
>   			}
>   		}
>   
> +	if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
> +	    info.value_size != 8)
> +		p_info("Warning: cannot read values from %s map with value_size != 8",
> +		       map_type_name[info.type]);
>   	while (true) {
>   		err = bpf_map_get_next_key(fd, prev_key, key);
>   		if (err) {
> 

All good for me now, thanks a lot!

Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>

Re: [PATCH v3 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Jakub Kicinski]

On Mon, 15 Apr 2019 16:15:35 +0900, Benjamin Poirier wrote:
> Commit bf598a8f0f77 ("bpftool: Improve handling of ENOENT on map dumps")
> used print_entry_plain() in case of ENOENT. However, that commit introduces
> dead code. Per-cpu maps are zero-filled. When reading them, it's all or
> nothing. There will never be a case where some cpus have an entry and
> others don't.
> 
> The truth is that ENOENT is an error case. Use print_entry_error() to
> output the desired message. That function's "value" parameter is also
> renamed to indicate that we never use it for an actual map value.
> 
> The output format is unchanged.
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Re: [PATCH v3 bpf-next 2/2] bpftool: Improve handling of ENOSPC on reuseport_array map dumps

[Jakub Kicinski]

On Mon, 15 Apr 2019 16:15:36 +0900, Benjamin Poirier wrote:
> avoids outputting a series of
> 	value:
> 	No space left on device
> 
> The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
> only return it if the map was created with value_size = 8. There's nothing
> bpftool can do about it. Instead of repeating this error for every key in
> the map, print an explanatory warning and a specialized error.
> 
> example before:
> key: 00 00 00 00
> value:
> No space left on device
> key: 01 00 00 00
> value:
> No space left on device
> key: 02 00 00 00
> value:
> No space left on device
> Found 0 elements
> 
> example after:
> Warning: cannot read values from reuseport_sockarray map with value_size != 8
> key: 00 00 00 00  value: <cannot read>
> key: 01 00 00 00  value: <cannot read>
> key: 02 00 00 00  value: <cannot read>
> Found 0 elements
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Thank you!!

Re: [PATCH v3 bpf-next 1/2] bpftool: Use print_entry_error() in case of ENOENT when dumping

[Daniel Borkmann]

On 04/15/2019 09:15 AM, Benjamin Poirier wrote:
> Commit bf598a8f0f77 ("bpftool: Improve handling of ENOENT on map dumps")
> used print_entry_plain() in case of ENOENT. However, that commit introduces
> dead code. Per-cpu maps are zero-filled. When reading them, it's all or
> nothing. There will never be a case where some cpus have an entry and
> others don't.
> 
> The truth is that ENOENT is an error case. Use print_entry_error() to
> output the desired message. That function's "value" parameter is also
> renamed to indicate that we never use it for an actual map value.
> 
> The output format is unchanged.
> 
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>

Both applied, thanks!