Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:
> Let's add a check for EOPNOTSUPP error when map lookup is failed.
> Also in case map doesn't support lookup, the output of map dump is
> changed from "can't lookup element" to "lookup not supported for
> this map".
> 
> Patch adds function print_entry_error() function to print the error
> value.
> 
> Following example dumps a map which does not support lookup.
> 
> Output before:
> root# bpftool map -jp dump id 40
> [
>     "key": ["0x0a","0x00","0x00","0x00"
>     ],
>     "value": {
>         "error": "can\'t lookup element"
>     },
>     "key": ["0x0b","0x00","0x00","0x00"
>     ],
>     "value": {
>         "error": "can\'t lookup element"
>     }
> ]
> 
> root# bpftool map dump id 40
> can't lookup element with key:
> 0a 00 00 00
> can't lookup element with key:
> 0b 00 00 00
> Found 0 elements
> 
> Output after changes:
> root# bpftool map dump -jp  id 45
> [
>     "key": ["0x0a","0x00","0x00","0x00"
>     ],
>     "value": {
>         "error": "lookup not supported for this map"
>     },
>     "key": ["0x0b","0x00","0x00","0x00"
>     ],
>     "value": {
>         "error": "lookup not supported for this map"
>     }
> ]
> 
> root# bpftool map dump id 45
> key:
> 0a 00 00 00
> value:
> lookup not supported for this map
> key:
> 0b 00 00 00
> value:
> lookup not supported for this map
> Found 0 elements

Nice improvement, thanks for the changes!  I wonder what your thoughts
would be on just printing some form of "lookup not supported for this
map" only once?  It seems slightly like repeated information - if
lookup is not supported for one key it likely won't be for other keys
too, so we could shorten the output.  Would that make sense?

> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
>  tools/bpf/bpftool/main.h |  5 +++++
>  tools/bpf/bpftool/map.c  | 35 ++++++++++++++++++++++++++++++-----
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 40492cdc4e53..1a8c683f949b 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -46,6 +46,11 @@
>  
>  #include "json_writer.h"
>  
> +#define ERR_CANNOT_LOOKUP \
> +	"can't lookup element"
> +#define ERR_LOOKUP_NOT_SUPPORTED \
> +	"lookup not supported for this map"

Do we need these?  Are we going to reused them in more parts of the
code?

>  #define ptr_to_u64(ptr)	((__u64)(unsigned long)(ptr))
>  
>  #define NEXT_ARG()	({ argc--; argv++; if (argc < 0) usage(); })
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 284e12a289c0..2faccd2098c9 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -333,6 +333,25 @@ 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,
> +			      char *value)
> +{
> +	bool single_line, break_names;
> +	int value_size = strlen(value);

nit: order variables declaration lines to shortest, please.

> +
> +	break_names = info->key_size > 16 || value_size > 16;
> +	single_line = info->key_size + value_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("\n");
> +}
> +
>  static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>  			      unsigned char *value)
>  {
> @@ -660,6 +679,8 @@ static int dump_map_elem(int fd, void *key, void *value,
>  			 json_writer_t *btf_wtr)
>  {
>  	int num_elems = 0;
> +	int lookup_errno;
> +	char *errstr;

nit: const char?

>  
>  	if (!bpf_map_lookup_elem(fd, key, value)) {
>  		if (json_output) {