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, 9 Jul 2018 21:06:09 -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 v3 3/3] bpf: btf: print map dump and lookup
 with btf info
Message-ID: <20180709210609.284b29f7@cakuba.lan>
In-Reply-To: <20180708203336.570589830@fb.com>
References: <20180708203002.543403467@fb.com>
        <20180708203336.570589830@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 Sun, 8 Jul 2018 13:30:05 -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>

> +static struct btf *get_btf(struct bpf_map_info *map_info)
> +{
> +	struct bpf_btf_info btf_info = { 0 };
> +	__u32 len = sizeof(btf_info);
> +	struct btf *btf = NULL;
> +	__u32 last_size;
> +	int btf_fd;
> +	void *ptr;
> +	int err;
> +
> +	btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
> +	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.
> +	 */
> +	btf_info.btf_size = 4096;
> +	last_size = btf_info.btf_size;
> +	ptr = malloc(last_size);
> +	if (!ptr) {
> +		p_err("unable to allocate memory for debug info");
> +		goto exit_free;

I don't think we can continue working after a p_err() call :S
Or p_info() for that matter.  Something else may call p_err() again and
we'll end up with multiple "error" members in JSON.

Could you return an error and make the callers fail where you do goto
exit_free?  The case there is no BTF is okay to return NULL, but other
cases should really not happen, and I think it's OK to just error out
completely.

> +	}
> +
> +	bzero(ptr, last_size);
> +	btf_info.btf = ptr_to_u64(ptr);
> +	err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
> +
> +	if (!err && btf_info.btf_size > last_size) {
> +		void *temp_ptr;
> +
> +		last_size = btf_info.btf_size;
> +		temp_ptr = realloc(ptr, last_size);
> +		if (!temp_ptr) {
> +			p_err("unable to re-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);
> +	}
> +
> +	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;
> +	}
> +
> +	btf = btf__new((__u8 *)btf_info.btf,
> +		       btf_info.btf_size, NULL);
> +	if (IS_ERR(btf)) {
> +		p_info("error when initialising btf: %s\n",
> +		       strerror(PTR_ERR(btf)));
> +		btf = NULL;
> +	}
> +
> +exit_free:
> +	close(btf_fd);
> +	free(ptr);
> +
> +	return btf;
> +}

> @@ -549,9 +681,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,
> +					};

nit: new line missing here and in another place

> +					do_dump_btf(&d, &info, key, value);
> +				} else {