From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed Date: Wed, 19 Sep 2018 08:29:54 -0700 Message-ID: <20180919082954.50a827a0@cakuba.netronome.com> References: <20180919075143.9308-1-bhole_prashant_q7@lab.ntt.co.jp> <20180919075143.9308-5-bhole_prashant_q7@lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Alexei Starovoitov , Daniel Borkmann , Quentin Monnet , "David S . Miller" , netdev@vger.kernel.org To: Prashant Bhole Return-path: Received: from mail-qt0-f193.google.com ([209.85.216.193]:34520 "EHLO mail-qt0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731957AbeISVIh (ORCPT ); Wed, 19 Sep 2018 17:08:37 -0400 Received: by mail-qt0-f193.google.com with SMTP id m13-v6so5511569qth.1 for ; Wed, 19 Sep 2018 08:30:10 -0700 (PDT) In-Reply-To: <20180919075143.9308-5-bhole_prashant_q7@lab.ntt.co.jp> Sender: netdev-owner@vger.kernel.org List-ID: 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 > --- > 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) {