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...@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 ;)
