Message-ID: <86ae5059-54c8-d078-4f6b-b212285dbfec@netronome.com>
Date:   Thu, 21 Jun 2018 11:24:59 +0100
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Okash Khawaja <osk@...com>, 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>
Cc:     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 Okash,

Thanks for the patch! Please find some nitpicks inline 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?

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

Powered by blists - more mailing lists