Messages in this thread Patch in this message |  | | Date | Mon, 2 Jul 2018 22:29:16 -0700 | | From | Jakub Kicinski <> | | Subject | Re: [PATCH bpf-next v2 3/3] bpf: btf: print map dump and lookup with btf info |
| |
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; > } >
|  |