Re: [RFC bpf-next 3/4] tools/bpf: bpftool, split the function do_dump()

[Prashant Bhole]

On 9/20/2018 12:26 AM, Jakub Kicinski wrote:

On Wed, 19 Sep 2018 16:51:42 +0900, Prashant Bhole wrote:

+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;
+
+   if (!bpf_map_lookup_elem(fd, key, value)) {
+   if (json_output) {
+   print_entry_json(map_info, key, value, btf);
+   } else {
+   if (btf) {
+   struct btf_dumper d = {
+   .btf = btf,
+   .jw = btf_wtr,
+   .is_plain_text = true,
+   };
+
+   do_dump_btf(&d, map_info, key, value);
+   } else {
+   print_entry_plain(map_info, key, value);
+   }
+   num_elems++;
+   }
+   goto out;
+   }
+
+   /* lookup error handling */
+   if (map_is_map_of_maps(map_info->type) ||
+   map_is_map_of_progs(map_info->type))
+   goto out;
+


nit: why not just return?  the goto seems to only do a return anyway,
  is this suggested by some coding style?  Is it to help the
  compiler?  I see people do this from time to time..


Thanks for reviewing. I agree, goto and the label isn't needed. I will 
fix it.


-Prashant


[...]


+out:
+   return num_elems;

Re: [RFC bpf-next 3/4] tools/bpf: bpftool, split the function do_dump()

[Jakub Kicinski]

On Wed, 19 Sep 2018 16:51:42 +0900, Prashant Bhole wrote:
> +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;
> +
> + if (!bpf_map_lookup_elem(fd, key, value)) {
> + if (json_output) {
> + print_entry_json(map_info, key, value, btf);
> + } else {
> + if (btf) {
> + struct btf_dumper d = {
> + .btf = btf,
> + .jw = btf_wtr,
> + .is_plain_text = true,
> + };
> +
> + do_dump_btf(&d, map_info, key, value);
> + } else {
> + print_entry_plain(map_info, key, value);
> + }
> + num_elems++;
> + }
> + goto out;
> + }
> +
> + /* lookup error handling */
> + if (map_is_map_of_maps(map_info->type) ||
> + map_is_map_of_progs(map_info->type))
> + goto out;
> +

nit: why not just return?  the goto seems to only do a return anyway,
 is this suggested by some coding style?  Is it to help the
 compiler?  I see people do this from time to time..

[...]

> +out:
> + return num_elems;

[RFC bpf-next 3/4] tools/bpf: bpftool, split the function do_dump()

[Prashant Bhole]

do_dump() function in bpftool/map.c has deep indentations. In order
to reduce deep indent, let's move element printing code out of
do_dump() into dump_map_elem() function.

Signed-off-by: Prashant Bhole 
---
 tools/bpf/bpftool/map.c | 83 -
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index af8ad32fa6e9..284e12a289c0 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -655,6 +655,54 @@ static int do_show(int argc, char **argv)
return errno == ENOENT ? 0 : -1;
 }
 
+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;
+
+   if (!bpf_map_lookup_elem(fd, key, value)) {
+   if (json_output) {
+   print_entry_json(map_info, key, value, btf);
+   } else {
+   if (btf) {
+   struct btf_dumper d = {
+   .btf = btf,
+   .jw = btf_wtr,
+   .is_plain_text = true,
+   };
+
+   do_dump_btf(&d, map_info, key, value);
+   } else {
+   print_entry_plain(map_info, key, value);
+   }
+   num_elems++;
+   }
+   goto out;
+   }
+
+   /* lookup error handling */
+   if (map_is_map_of_maps(map_info->type) ||
+   map_is_map_of_progs(map_info->type))
+   goto out;
+
+   if (json_output) {
+   jsonw_name(json_wtr, "key");
+   print_hex_data_json(key, map_info->key_size);
+   jsonw_name(json_wtr, "value");
+   jsonw_start_object(json_wtr);
+   jsonw_string_field(json_wtr, "error",
+  "can't lookup element");
+   jsonw_end_object(json_wtr);
+   } else {
+   p_info("can't lookup element with key: ");
+   fprint_hex(stderr, key, map_info->key_size, " ");
+   fprintf(stderr, "\n");
+   }
+out:
+   return num_elems;
+}
+
 static int do_dump(int argc, char **argv)
 {
struct bpf_map_info info = {};
@@ -710,40 +758,7 @@ static int do_dump(int argc, char **argv)
err = 0;
break;
}
-
-   if (!bpf_map_lookup_elem(fd, key, value)) {
-   if (json_output)
-   print_entry_json(&info, key, value, btf);
-   else
-   if (btf) {
-   struct btf_dumper d = {
-   .btf = btf,
-   .jw = btf_wtr,
-   .is_plain_text = true,
-   };
-
-   do_dump_btf(&d, &info, key, value);
-   } else {
-   print_entry_plain(&info, key, value);
-   }
-   num_elems++;
-   } else if (!map_is_map_of_maps(info.type) &&
-  !map_is_map_of_progs(info.type)) {
-   if (json_output) {
-   jsonw_name(json_wtr, "key");
-   print_hex_data_json(key, info.key_size);
-   jsonw_name(json_wtr, "value");
-   jsonw_start_object(json_wtr);
-   jsonw_string_field(json_wtr, "error",
-  "can't lookup element");
-   jsonw_end_object(json_wtr);
-   } else {
-   p_info("can't lookup element with key: ");
-   fprint_hex(stderr, key, info.key_size, " ");
-   fprintf(stderr, "\n");
-   }
-   }
-
+   num_elems += dump_map_elem(fd, key, value, &info, btf, btf_wtr);
prev_key = key;
}
 
-- 
2.17.1