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: Thu, 20 Sep 2018 08:59:15 -0700 Message-ID: <20180920085915.27925590@cakuba.netronome.com> References: <20180919075143.9308-1-bhole_prashant_q7@lab.ntt.co.jp> <20180919075143.9308-5-bhole_prashant_q7@lab.ntt.co.jp> <20180919082954.50a827a0@cakuba.netronome.com> <6fb405e9-fe02-17c6-90e6-cf9c73ca7693@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-f195.google.com ([209.85.216.195]:44977 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727229AbeITVn2 (ORCPT ); Thu, 20 Sep 2018 17:43:28 -0400 Received: by mail-qt0-f195.google.com with SMTP id k38-v6so8957021qtk.11 for ; Thu, 20 Sep 2018 08:59:20 -0700 (PDT) In-Reply-To: <6fb405e9-fe02-17c6-90e6-cf9c73ca7693@lab.ntt.co.jp> Sender: netdev-owner@vger.kernel.org List-ID: 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 > >> --- > >> 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 ;)