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

[Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>]

On 9/21/2018 12:59 AM, Jakub Kicinski wrote:
> 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 ;)

Sorry for the late reply.
In v2 I have removed the error strings altogether. As you suggested 
output will be strerror(). Also added verifier tests as Alexei 
suggested. I am sending the RFC-v2 series soon.
Thanks.

-Prashant