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.
>
> >> + if (map_is_map_of_maps(map_info->type) ||
> >> + map_is_map_of_progs(map_info->type))
> >> + return;
> >> +
> >> + if (json_output) {
> >> + jsonw_start_object(json_wtr); /* entry */
> >> + jsonw_name(json_wtr, "key");
> >> + print_hex_data_json(key, map_info->key_size);
> >> + jsonw_name(json_wtr, "value");
> >> + jsonw_start_object(json_wtr); /* error */
> >> + jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
> >> + jsonw_end_object(json_wtr); /* error */
> >> + jsonw_end_object(json_wtr); /* entry */
> >> + } else {
> >> + const char *msg = NULL;
> >> +
> >> + if (lookup_errno == ENOENT)
> >> + msg = "<no entry>";
> >> + else if (lookup_errno == ENOSPC &&
> >> + map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
> >> + msg = "<cannot read>";
> >> +
> >> + print_entry_error_msg(map_info, key,
> >> + msg ? : strerror(lookup_errno));
> >> + }
> >> +}
> >> +
> >
> > [...]
> >
>
