From mboxrd@z Thu Jan 1 00:00:00 1970 From: Quentin Monnet Subject: Re: [PATCH bpf-next 3/3] bpf: btf: json print map dump with btf info Date: Thu, 21 Jun 2018 11:24:59 +0100 Message-ID: <86ae5059-54c8-d078-4f6b-b212285dbfec@netronome.com> References: <20180620203051.223156973@fb.com> <20180620203703.208599277@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: netdev@vger.kernel.org, kernel-team@fb.com, linux-kernel@vger.kernel.org To: Okash Khawaja , Daniel Borkmann , Martin KaFai Lau , Alexei Starovoitov , Yonghong Song , Jakub Kicinski , "David S. Miller" Return-path: In-Reply-To: <20180620203703.208599277@fb.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org Hi Okash, Thanks for the patch! Please find some nitpicks inline below. 2018-06-20 13:30 UTC-0700 ~ Okash Khawaja > This patch modifies `bpftool map dump [-j|-p] 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 > Acked-by: Martin KaFai Lau > > --- > 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 > #include > #include > +#include > > #include > > +#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