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

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

On Thu, 20 Sep 2018 14:04:19 +0900, Prashant Bhole wrote:
> On 9/20/2018 12:29 AM, Jakub Kicinski wrote:
> > 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?  
> 
> These are used only once. These can be used in do_lookup(). Currently 
> do_lookup() prints strerror(errno) when lookup is failed. Shall I change 
> that do_lookup() output?

I actually prefer to stick to strerror(), the standard errors more
clearly correlate with what happened in my mind (i.e. "Operation not
supported" == kernel sent EOPNOTSUPP).   strerror() may also print in
local language if translation/localization matters.

We could even use strerr() in dump_map_elem() but up to you.  The one
in do_lookup() I'd prefer to leave be ;)