Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Sep 9, 2020 at 9:59 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On 09/09/2020 17:46, Andrii Nakryiko wrote:
> > On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> On 09/09/2020 17:30, Andrii Nakryiko wrote:
> >>> On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>>>
> >>>> On 09/09/2020 04:25, Andrii Nakryiko wrote:
> >>>>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>>>>>
> >>>>>> The function used to dump a map entry in bpftool is a bit difficult to
> >>>>>> follow, as a consequence to earlier refactorings. There is a variable
> >>>>>> ("num_elems") which does not appear to be necessary, and the error
> >>>>>> handling would look cleaner if moved to its own function. Let's clean it
> >>>>>> up. No functional change.
> >>>>>>
> >>>>>> v2:
> >>>>>> - v1 was erroneously removing the check on fd maps in an attempt to get
> >>>>>>   support for outer map dumps. This is already working. Instead, v2
> >>>>>>   focuses on cleaning up the dump_map_elem() function, to avoid
> >>>>>>   similar confusion in the future.
> >>>>>>
> >>>>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> >>>>>> ---
> >>>>>>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
> >>>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
> >>>>>>
> >>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> >>>>>> index bc0071228f88..c8159cb4fb1e 100644
> >>>>>> --- a/tools/bpf/bpftool/map.c
> >>>>>> +++ b/tools/bpf/bpftool/map.c
> >>>>>> @@ -213,8 +213,9 @@ 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,
> >>>>>> -                             const char *error_msg)
> >>>>>> +static void
> >>>>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
> >>>>>> +                     const char *error_msg)
> >>>>>>  {
> >>>>>>         int msg_size = strlen(error_msg);
> >>>>>>         bool single_line, break_names;
> >>>>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> >>>>>>         printf("\n");
> >>>>>>  }
> >>>>>>
> >>>>>> +static void
> >>>>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
> >>>>>> +{
> >>>>>> +       /* For prog_array maps or arrays of maps, failure to lookup the value
> >>>>>> +        * means there is no entry for that key. Do not print an error message
> >>>>>> +        * in that case.
> >>>>>> +        */
> >>>>>
> >>>>> this is the case when error is ENOENT, all the other ones should be
> >>>>> treated the same, no?
> >>>>
> >>>> Do you mean all map types should be treated the same? If so, I can
> >>>> remove the check below, as in v1. Or do you mean there is a missing
> >>>> check on the error value? In which case I can extend this check to
> >>>> verify we have ENOENT.
> >>>
> >>> The former, probably. I don't see how map-in-map is different for
> >>> lookups and why it needs special handling.
> >>
> >> I didn't find a particular reason in the logs. My guess is that they may
> >> be more likely to have "empty" entries than other types, and that it
> >> might be more difficult to spot the existing entries in the middle of a
> >> list of "<no entry>" messages.
> >>
> >> But I agree, let's get rid of this special case and have the same output
> >> for all types. I'll respin.
> >
> > Oh, wait, I think what I had in mind is to special case ENOENT for
> > map-in-map and just skip those. So yeah, sorry, there is still a bit
> > of a special handling, but **only** for -ENOENT. When I was replying I
> > forgot bpftool emits "<no entry>" for each -ENOENT by default.
>
> So do you prefer me to extend the check with errno == -ENOENT? Or shall
> I remove it entirely and just have the "<no entry>" messages like for
> the other map types?

Extend with -ENOENT check.

>
> Quentin