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?
> #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) {
