2019-04-11 17:27 UTC+0900 ~ Benjamin Poirier <bpoir...@suse.com>
> avoids outputting a series of
> value:
> No space left on device
>
> The value itself is not wrong but bpf_fd_reuseport_array_lookup_elem() can
> only return it if the map was created with value_size = 8. There's nothing
> bpftool can do about it. Instead of repeating this error for every key in
> the map, print the error once, print an explanatory message and exit.
>
> Signed-off-by: Benjamin Poirier <bpoir...@suse.com>
> ---
>
> Note that this will lead to a merge conflict if the patch "bpftool: Fix
> errno variable usage" is merged in the bpf branch:
>
> <<<<<<< HEAD
> if (lookup_errno == ENOENT)
> =======
> if (errno == ENOENT) {
>>>>>>>> bpf-next
>
> The resolution should be trivial:
> if (lookup_errno == ENOENT) {
>
> tools/bpf/bpftool/map.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index e96903078991..bb648e42923c 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -684,7 +684,6 @@ static int dump_map_elem(int fd, void *key, void *value,
> struct bpf_map_info *map_info, struct btf *btf,
> json_writer_t *btf_wtr)
> {
> - int num_elems = 0;
> int lookup_errno;
>
> if (!bpf_map_lookup_elem(fd, key, value)) {
> @@ -702,9 +701,8 @@ static int dump_map_elem(int fd, void *key, void *value,
> } else {
> print_entry_plain(map_info, key, value);
> }
> - num_elems++;
> }
> - return num_elems;
> + return 1;
> }
>
> /* lookup error handling */
> @@ -722,11 +720,13 @@ static int dump_map_elem(int fd, void *key, void *value,
> jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
> jsonw_end_object(json_wtr);
> } else {
> - if (errno == ENOENT)
> + if (errno == ENOENT) {
> print_entry_plain(map_info, key, NULL);
> - else
> + } else {
> print_entry_error(map_info, key,
> strerror(lookup_errno));
> + return -lookup_errno;
Hi, thanks! One comment: if we return -lookup_errno here...
> + }
> }
>
> return 0;
> @@ -787,7 +787,16 @@ static int do_dump(int argc, char **argv)
> err = 0;
> break;
> }
> - num_elems += dump_map_elem(fd, key, value, &info, btf, btf_wtr);
> + err = dump_map_elem(fd, key, value, &info, btf, btf_wtr);
> + /* bpf_fd_reuseport_array_lookup_elem() can only return a
> + * value if the map's value_size == 8
> + */
> + if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
> + info.value_size != 8 && err == -ENOSPC) {
> + p_err("cannot dump REUSEPORT_SOCKARRAY map with
> value_size != 8");
> + goto exit_free;
> + }
> + num_elems += err;
... I think we can have a negative err value here (if we're not in the
case of reuseport_sockarray with wrong map size)? In that case, the
function could print something like "Found -1 elements"?
So maybe update num_elems only if err is positive? Although I'd like
even better if we could somehow find a way to move this error handling
along with the rest of it in dump_map_elem(), and avoid making that
function return either a number of elements or an error depending on the
result from the lookup.
Best regards,
Quentin
