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: Tue, 2 Oct 2018 14:33:40 +0900 Message-ID: <7d104e2a-2292-3fc3-b31a-2f03e987aa5e@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> <6fb405e9-fe02-17c6-90e6-cf9c73ca7693@lab.ntt.co.jp> <20180920085915.27925590@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Daniel Borkmann , Quentin Monnet , "David S . Miller" , netdev@vger.kernel.org To: Jakub Kicinski , Alexei Starovoitov Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:52750 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726374AbeJBMQJ (ORCPT ); Tue, 2 Oct 2018 08:16:09 -0400 In-Reply-To: <20180920085915.27925590@cakuba.netronome.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >>>> --- >>>> 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