From mboxrd@z Thu Jan 1 00:00:00 1970 From: Prashant Bhole Subject: Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed Date: Thu, 20 Sep 2018 14:04:19 +0900 Message-ID: <6fb405e9-fe02-17c6-90e6-cf9c73ca7693@lab.ntt.co.jp> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Alexei Starovoitov , Daniel Borkmann , Quentin Monnet , "David S . Miller" , netdev@vger.kernel.org To: Jakub Kicinski Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:54656 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725725AbeITKqi (ORCPT ); Thu, 20 Sep 2018 06:46:38 -0400 In-Reply-To: <20180919082954.50a827a0@cakuba.netronome.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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? -Prashant