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.
Thanks again,
Quentin
