lkml.org 
[lkml]   [2018]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH bpf-next 3/3] bpf: btf: json print map dump with btf info
From
Date
Hi Okash,

Thanks for the patch! Please find some nitpicks inline below.

2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <osk@fb.com>
> This patch modifies `bpftool map dump [-j|-p] id <map-id>` to json-
> print and pretty-json-print map dump. It calls btf_dumper introduced in
> previous patch to accomplish this.
>
> The patch only prints debug info when -j or -p flags are supplied. Then
> too, if the map has associated btf data loaded. Otherwise the usual
> debug-less output is printed.
>
> Signed-off-by: Okash Khawaja <osk@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> ---
> tools/bpf/bpftool/map.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -43,9 +43,13 @@
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> +#include <linux/err.h>
>
> #include <bpf.h>
>
> +#include "json_writer.h"
> +#include "btf.h"
> +#include "btf_dumper.h"
> #include "main.h"
>
> static const char * const map_type_name[] = {
> @@ -508,6 +512,83 @@ static int do_show(int argc, char **argv
> return errno == ENOENT ? 0 : -1;
> }
>
> +
> +static int do_dump_btf(struct btf *btf, struct bpf_map_info *map_info,
> + void *key, void *value)

Nit: Please align the second line on the opening parenthesis.

> +{
> + int ret;
> +
> + jsonw_start_object(json_wtr);
> + jsonw_name(json_wtr, "key");
> +
> + ret = btf_dumper_type(btf, json_wtr, map_info->btf_key_type_id, key);
> + if (ret)
> + goto out;
> +
> + jsonw_end_object(json_wtr);
> +
> + jsonw_start_object(json_wtr);
> + jsonw_name(json_wtr, "value");
> +
> + ret = btf_dumper_type(btf, json_wtr, map_info->btf_value_type_id,
> + value);

Same comment.

> +
> +out:
> + /* end of root object */
> + jsonw_end_object(json_wtr);

This is not the root JSON object, which is not produced in that
function, so I find the comment misleading.

I also find it confusing that it closes the first JSON object of this
function if there is an error, but the second if "btf_dumper_type()"
succeeds. What about the following: closing the first object in all
cases, before evaluating the value of "ret", and if "ret" is non-null
returning immediately; and completely removing the "goto" from this
function?

> +
> + return ret;
> +}
> +
> +static struct btf *get_btf(struct bpf_map_info *map_info)
> +{
> + int btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
> + struct bpf_btf_info btf_info = { 0 };
> + __u32 len = sizeof(btf_info);
> + uint32_t last_size;
> + int err;
> + struct btf *btf = NULL;
> + void *ptr = NULL, *temp_ptr;

Nit: please sort declarations in reverse-Christmas-tree order.

> +
> + if (btf_fd < 0)
> + return NULL;
> +
> + btf_info.btf_size = 4096;
> + do {
> + last_size = btf_info.btf_size;
> + temp_ptr = realloc(ptr, last_size);
> + if (!temp_ptr) {
> + p_err("unable allocate memory for debug info.");

"unable *to* allocate"?
(Also most other error messages do not end with a period, but here this
is just me being fussy.)

> + goto exit_free;
> + }
> +
> + ptr = temp_ptr;
> + bzero(ptr, last_size);
> + btf_info.btf = ptr_to_u64(ptr);
> + err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
> + } while (!err && btf_info.btf_size > last_size && last_size == 4096);

If I understand correctly, the first time you try to retrieve up to 4096
bytes, and if the btf_info is larger than this, you try a second time
with the size returned in btf_info.btf_size instead. I don't find it
intuitive (but maybe this is just me?), do you think you could add a
comment above this bloc maybe?

> +
> + if (err || btf_info.btf_size > last_size) {
> + p_info("can't get btf info. debug info won't be displayed. error: %s",
> + err ? strerror(errno) : "exceeds size retry");

Nit: Please align the second line on the opening parenthesis.

> + goto exit_free;
> + }
> +
> + btf = btf__new((uint8_t *) btf_info.btf,

Nit: No space between the cast and the name of the variable.

> + btf_info.btf_size, NULL);

Same remark on parenthesis here...

> + if (IS_ERR(btf)) {
> + printf("error when initialising btf: %s\n",
> + strerror(PTR_ERR(btf)));

... and here.

> + btf = NULL;
> + }
> +
> +exit_free:
> + close(btf_fd);
> + free(ptr);
> +
> + return btf;
> +}
> +
> static int do_dump(int argc, char **argv)
> {
> void *key, *value, *prev_key;
> @@ -516,6 +597,7 @@ static int do_dump(int argc, char **argv
> __u32 len = sizeof(info);
> int err;
> int fd;
> + struct btf *btf = NULL;

Reverse-Christmas-tree order, please.

>
> if (argc != 2)
> usage();
> @@ -538,6 +620,8 @@ static int do_dump(int argc, char **argv
> goto exit_free;
> }
>
> + btf = get_btf(&info);
> +
> prev_key = NULL;
> if (json_output)
> jsonw_start_array(json_wtr);
> @@ -550,9 +634,12 @@ static int do_dump(int argc, char **argv
> }
>
> if (!bpf_map_lookup_elem(fd, key, value)) {
> - if (json_output)
> - print_entry_json(&info, key, value);
> - else
> + if (json_output) {
> + if (btf)
> + do_dump_btf(btf, &info, key, value);
> + else
> + print_entry_json(&info, key, value);
> + } else
> print_entry_plain(&info, key, value);

Please add brackets around "print_entry_plain()" (to harmonise with the
"if" of the same bloc).

> } else {
> if (json_output) {
> @@ -584,6 +671,7 @@ exit_free:
> free(key);
> free(value);
> close(fd);
> + btf__free(btf);
>
> return err;
> }
>

Thanks,
Quentin

\
 
 \ /
  Last update: 2018-06-21 12:26    [W:0.187 / U:0.788 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site