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