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

From: Jakub Kicinski
Date: Tue Jul 03 2018 - 01:29:27 EST

Next message: Ravi Bangoria: "Re: [PATCH v5 06/10] Uprobes: Support SDT markers having reference count (semaphore)"
Previous message: Gautham R. Shenoy: "[PATCH] cpuidle:powernv: Add the CPUIDLE_FLAG_POLLING for snooze"
In reply to: Okash Khawaja: "[PATCH bpf-next v2 3/3] bpf: btf: print map dump and lookup with btf info"
Next in thread: Jakub Kicinski: "Re: [PATCH bpf-next v2 1/3] bpf: btf: export btf types and name by offset from lib"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxx>
>
> ---
> 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;
> }
>

Next message: Ravi Bangoria: "Re: [PATCH v5 06/10] Uprobes: Support SDT markers having reference count (semaphore)"
Previous message: Gautham R. Shenoy: "[PATCH] cpuidle:powernv: Add the CPUIDLE_FLAG_POLLING for snooze"
In reply to: Okash Khawaja: "[PATCH bpf-next v2 3/3] bpf: btf: print map dump and lookup with btf info"
Next in thread: Jakub Kicinski: "Re: [PATCH bpf-next v2 1/3] bpf: btf: export btf types and name by offset from lib"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]