On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> On 09/09/2020 17:30, Andrii Nakryiko wrote:
> > On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
> >>
> >> On 09/09/2020 04:25, Andrii Nakryiko wrote:
> >>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>>> ---
> >>>> 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.
>
> Thanks again,
> Quentin
