Re: [PATCH bpf-next v3 3/3] bpf: btf: print map dump and lookup with btf info

Jakub Kicinski
2018-07-10T04:06:31Z

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 {