Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 2 Jul 2018 22:29:16 -0700
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Okash Khawaja <osk@fb.com>
Cc:     Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Yonghong Song <yhs@fb.com>,
        "Quentin Monnet" <quentin.monnet@netronome.com>,
        "David S. Miller" <davem@davemloft.net>, <netdev@vger.kernel.org>,
        <kernel-team@fb.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 3/3] bpf: btf: print map dump and lookup
 with btf info
Message-ID: <20180702222916.295e8012@cakuba.netronome.com>
In-Reply-To: <20180702191324.683291855@fb.com>
References: <20180702183913.669030439@fb.com>
        <20180702191324.683291855@fb.com>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 2 Jul 2018 11:39:16 -0700, Okash Khawaja wrote:
> This patch augments the output of bpftool's map dump and map lookup
> commands to print data along side btf info, if the correspondin btf
> info is available. The outputs for each of  map dump and map lookup
> commands are augmented in two ways:
> 
> 1. when neither of -j and -p are supplied, btf-ful map data is printed
> whose aim is human readability. This means no commitments for json- or
> backward- compatibility.
> 
> 2. when either -j or -p are supplied, a new json object named
> "formatted" is added for each key-value pair. This object contains the
> same data as the key-value pair, but with btf info. "formatted" object
> promises json- and backward- compatibility. Below is a sample output.
> 
> $ bpftool map dump -p id 8
> [{
>         "key": ["0x0f","0x00","0x00","0x00"
>         ],
>         "value": ["0x03", "0x00", "0x00", "0x00", ...
>         ],
>         "formatted": {
>                 "key": 15,
>                 "value": {
>                         "int_field":  3,
>                         ...
>                 }
>         }
> }
> ]
> 
> This patch calls btf_dumper introduced in previous patch to accomplish
> the above. Indeed, btf-ful info is only displayed if btf data for the
> given map is available. Otherwise existing output is displayed as-is.
> 
> Signed-off-by: Okash Khawaja <osk@fb.com>
> 
> ---
>  tools/bpf/bpftool/map.c |  174 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 166 insertions(+), 8 deletions(-)
> 
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -41,9 +41,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"

Please keep the headers in alphabetical order.

>  static const char * const map_type_name[] = {
> @@ -148,8 +152,99 @@ int map_parse_fd_and_info(int *argc, cha
>  	return fd;
>  }
>  
> +static int do_dump_btf(const struct btf_dumper *d,
> +		       struct bpf_map_info *map_info, void *key,
> +		       void *value)
> +{
> +	int ret;
> +
> +	/* start of key-value pair */
> +	jsonw_start_object(d->jw);
> +
> +	jsonw_name(d->jw, "key");
> +
> +	ret = btf_dumper_type(d, map_info->btf_key_type_id, key);
> +	if (ret)
> +		return ret;

goto err_end_obj;

> +	jsonw_name(d->jw, "value");
> +
> +	ret = btf_dumper_type(d, map_info->btf_value_type_id, value);

err_end_obj:

> +	/* end of key-value pair */
> +	jsonw_end_object(d->jw);
> +
> +	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);

No failing functions in initializers please.

> +	struct bpf_btf_info btf_info = { 0 };
> +	__u32 len = sizeof(btf_info);
> +	void *ptr = NULL, *temp_ptr;
> +	struct btf *btf = NULL;
> +	uint32_t last_size;
> +	int err;
> +
> +	if (btf_fd < 0)
> +		return NULL;
> +
> +	/* we won't know btf_size until we call bpf_obj_get_info_by_fd(). so
> +	 * let's start with a sane default - 4KiB here - and resize it only if
> +	 * bpf_obj_get_info_by_fd() needs a bigger buffer. the do-while loop
> +	 * below should run a maximum of two iterations and that will be when
> +	 * we have to resize to a bigger buffer.
> +	 */
> +	btf_info.btf_size = 4096;
> +	do {
> +		last_size = btf_info.btf_size;
> +		temp_ptr = realloc(ptr, last_size);
> +		if (!temp_ptr) {
> +			p_err("unable to allocate memory for debug info");
> +			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 (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");
> +		goto exit_free;
> +	}

This "run me twice while handling realloc failure" loop seems very
unappealing.  Could you just open code this?

> +	btf = btf__new((uint8_t *)btf_info.btf,
> +		       btf_info.btf_size, NULL);
> +	if (IS_ERR(btf)) {
> +		printf("error when initialising btf: %s\n",
> +		       strerror(PTR_ERR(btf)));

printfs are not allowed, they break JSON.

> +		btf = NULL;
> +	}
> +
> +exit_free:
> +	close(btf_fd);
> +	free(ptr);
> +
> +	return btf;
> +}
> +
> +static json_writer_t *get_btf_writer(void)
> +{
> +	json_writer_t *jw = jsonw_new(stdout);
> +
> +	if (!jw)
> +		return NULL;
> +	jsonw_pretty(jw, true);
> +
> +	return jw;
> +}
> +
>  static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
> -			     unsigned char *value)
> +			     unsigned char *value, struct btf *btf)
>  {
>  	jsonw_start_object(json_wtr);
>  
> @@ -158,6 +253,15 @@ static void print_entry_json(struct bpf_
>  		print_hex_data_json(key, info->key_size);
>  		jsonw_name(json_wtr, "value");
>  		print_hex_data_json(value, info->value_size);
> +		if (btf) {
> +			struct btf_dumper d = {
> +				.btf = btf,
> +				.jw = json_wtr,
> +				.is_plain_text = false,
> +			};

Empty line after variables, please fix everywhere.

> +			jsonw_name(json_wtr, "formatted");
> +			do_dump_btf(&d, info, key, value);
> +		}
>  	} else {
>  		unsigned int i, n;
>  
> @@ -508,10 +612,12 @@ static int do_show(int argc, char **argv
>  
>  static int do_dump(int argc, char **argv)
>  {
> +	struct bpf_map_info info = {};
>  	void *key, *value, *prev_key;
>  	unsigned int num_elems = 0;
> -	struct bpf_map_info info = {};
>  	__u32 len = sizeof(info);
> +	json_writer_t *btf_wtr;
> +	struct btf *btf = NULL;
>  	int err;
>  	int fd;
>  
> @@ -537,8 +643,22 @@ static int do_dump(int argc, char **argv
>  	}
>  
>  	prev_key = NULL;
> +
> +	btf = get_btf(&info);
>  	if (json_output)
>  		jsonw_start_array(json_wtr);
> +	else
> +		if (btf) {
> +			btf_wtr = get_btf_writer();
> +			if (!btf_wtr) {
> +				p_info("failed to create json writer for btf. falling back to plain output");
> +				btf__free(btf);
> +				btf = NULL;
> +			} else {
> +				jsonw_start_array(btf_wtr);

Do we need to start array for non-JSON output?

> +			}
> +		}
> +
>  	while (true) {
>  		err = bpf_map_get_next_key(fd, prev_key, key);
>  		if (err) {
> @@ -549,9 +669,18 @@ 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);
> +				print_entry_json(&info, key, value, btf);
>  			else
> -				print_entry_plain(&info, key, value);
> +				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);
> +				}
>  		} else {
>  			if (json_output) {
>  				jsonw_name(json_wtr, "key");
> @@ -574,14 +703,19 @@ static int do_dump(int argc, char **argv
>  
>  	if (json_output)
>  		jsonw_end_array(json_wtr);
> -	else
> +	else if (btf) {
> +		jsonw_end_array(btf_wtr);
> +		jsonw_destroy(&btf_wtr);
> +	} else {
>  		printf("Found %u element%s\n", num_elems,
>  		       num_elems != 1 ? "s" : "");
> +	}
>  
>  exit_free:
>  	free(key);
>  	free(value);
>  	close(fd);
> +	btf__free(btf);
>  
>  	return err;
>  }
> @@ -637,6 +771,8 @@ static int do_lookup(int argc, char **ar
>  {
>  	struct bpf_map_info info = {};
>  	__u32 len = sizeof(info);
> +	json_writer_t *btf_wtr;
> +	struct btf *btf = NULL;
>  	void *key, *value;
>  	int err;
>  	int fd;
> @@ -662,10 +798,31 @@ static int do_lookup(int argc, char **ar
>  
>  	err = bpf_map_lookup_elem(fd, key, value);
>  	if (!err) {
> -		if (json_output)
> -			print_entry_json(&info, key, value);
> -		else
> +		btf = get_btf(&info);
> +		if (json_output) {
> +			print_entry_json(&info, key, value, btf);
> +		} else if (btf) {
> +			/* if here json_wtr wouldn't have been initialised,
> +			 * so let's create separate writer for btf
> +			 */
> +			btf_wtr = get_btf_writer();
> +			if (!btf_wtr) {
> +				p_info("failed to create json writer for btf. falling back to plain output");
> +				btf__free(btf);
> +				btf = NULL;
> +				print_entry_plain(&info, key, value);
> +			} else {
> +				struct btf_dumper d = {
> +					.btf = btf,
> +					.jw = btf_wtr,
> +					.is_plain_text = true,
> +				};
> +				do_dump_btf(&d, &info, key, value);
> +				jsonw_destroy(&btf_wtr);
> +			}
> +		} else {

This is way too much code in a if (!err) branch.

Please refactor so that err = bpf_map_lookup_elem(fd, key, value); is
followed by error handling and the actual printing is non-indented.

>  			print_entry_plain(&info, key, value);
> +		}
>  	} else if (errno == ENOENT) {
>  		if (json_output) {
>  			jsonw_null(json_wtr);
> @@ -682,6 +839,7 @@ exit_free:
>  	free(key);
>  	free(value);
>  	close(fd);
> +	btf__free(btf);
>  
>  	return err;
>  }
>