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

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

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

nit: new line missing here and in another place

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