Message-ID: <20180621142649.GC1176@w1t1fb>
Date:   Thu, 21 Jun 2018 15:26:51 +0100
From:   Okash Khawaja <osk@...com>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
CC:     Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Yonghong Song <yhs@...com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <kernel-team@...com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next 3/3] bpf: btf: json print map dump with btf info

Hi Quentin,

On Thu, Jun 21, 2018 at 11:24:59AM +0100, Quentin Monnet wrote:
> Hi Okash,
> 
> Thanks for the patch! Please find some nitpicks inline below.
Thanks for your feedback. All of it makes sense so I'll send v2 with
those changes. Couple of responses are inlined below.

> 
> 2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <osk@...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@...com>
> > Acked-by: Martin KaFai Lau <kafai@...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?
Code will be more intuitive that way so I'll re-organise it accordingly.

> 
> > +
> > +	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.)
I think it makes sense to be consistent. I'll remove the full stop.

> 
> > +			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?
Yes that is what this code is doing. I'll add comments explaining it.

> 
> > +
> > +	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

Powered by blists - more mailing lists