This patch modifies `bpftool map dump [-j|-p] id <map-id>` to json-
print and pretty-json-print map dump. It calls btf_dumper introduced in
previous patch to accomplish this.
The patch only prints debug info when -j or -p flags are supplied. Then
too, if the map has associated btf data loaded. Otherwise the usual
debug-less output is printed.
Signed-off-by: Okash Khawaja <[email protected]>
Acked-by: Martin KaFai Lau <[email protected]>
---
tools/bpf/bpftool/map.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 91 insertions(+), 3 deletions(-)
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -43,9 +43,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"
static const char * const map_type_name[] = {
@@ -508,6 +512,83 @@ static int do_show(int argc, char **argv
return errno == ENOENT ? 0 : -1;
}
+
+static int do_dump_btf(struct btf *btf, struct bpf_map_info *map_info,
+ void *key, void *value)
+{
+ int ret;
+
+ jsonw_start_object(json_wtr);
+ jsonw_name(json_wtr, "key");
+
+ ret = btf_dumper_type(btf, json_wtr, map_info->btf_key_type_id, key);
+ if (ret)
+ goto out;
+
+ jsonw_end_object(json_wtr);
+
+ jsonw_start_object(json_wtr);
+ jsonw_name(json_wtr, "value");
+
+ ret = btf_dumper_type(btf, json_wtr, map_info->btf_value_type_id,
+ value);
+
+out:
+ /* end of root object */
+ jsonw_end_object(json_wtr);
+
+ 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);
+ struct bpf_btf_info btf_info = { 0 };
+ __u32 len = sizeof(btf_info);
+ uint32_t last_size;
+ int err;
+ struct btf *btf = NULL;
+ void *ptr = NULL, *temp_ptr;
+
+ if (btf_fd < 0)
+ return NULL;
+
+ btf_info.btf_size = 4096;
+ do {
+ last_size = btf_info.btf_size;
+ temp_ptr = realloc(ptr, last_size);
+ if (!temp_ptr) {
+ p_err("unable 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;
+ }
+
+ 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)));
+ btf = NULL;
+ }
+
+exit_free:
+ close(btf_fd);
+ free(ptr);
+
+ return btf;
+}
+
static int do_dump(int argc, char **argv)
{
void *key, *value, *prev_key;
@@ -516,6 +597,7 @@ static int do_dump(int argc, char **argv
__u32 len = sizeof(info);
int err;
int fd;
+ struct btf *btf = NULL;
if (argc != 2)
usage();
@@ -538,6 +620,8 @@ static int do_dump(int argc, char **argv
goto exit_free;
}
+ btf = get_btf(&info);
+
prev_key = NULL;
if (json_output)
jsonw_start_array(json_wtr);
@@ -550,9 +634,12 @@ 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);
- else
+ if (json_output) {
+ if (btf)
+ do_dump_btf(btf, &info, key, value);
+ else
+ print_entry_json(&info, key, value);
+ } else
print_entry_plain(&info, key, value);
} else {
if (json_output) {
@@ -584,6 +671,7 @@ exit_free:
free(key);
free(value);
close(fd);
+ btf__free(btf);
return err;
}
> On Jun 20, 2018, at 1:30 PM, Okash Khawaja <[email protected]> wrote:
>
> This patch modifies `bpftool map dump [-j|-p] id <map-id>` to json-
> print and pretty-json-print map dump. It calls btf_dumper introduced in
> previous patch to accomplish this.
>
> The patch only prints debug info when -j or -p flags are supplied. Then
> too, if the map has associated btf data loaded. Otherwise the usual
> debug-less output is printed.
>
> Signed-off-by: Okash Khawaja <[email protected]>
> Acked-by: Martin KaFai Lau <[email protected]>
>
> ---
> tools/bpf/bpftool/map.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -43,9 +43,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"
>
> static const char * const map_type_name[] = {
> @@ -508,6 +512,83 @@ static int do_show(int argc, char **argv
> return errno == ENOENT ? 0 : -1;
> }
>
> +
> +static int do_dump_btf(struct btf *btf, struct bpf_map_info *map_info,
> + void *key, void *value)
> +{
> + int ret;
> +
> + jsonw_start_object(json_wtr);
> + jsonw_name(json_wtr, "key");
> +
> + ret = btf_dumper_type(btf, json_wtr, map_info->btf_key_type_id, key);
> + if (ret)
> + goto out;
> +
> + jsonw_end_object(json_wtr);
> +
> + jsonw_start_object(json_wtr);
> + jsonw_name(json_wtr, "value");
> +
> + ret = btf_dumper_type(btf, json_wtr, map_info->btf_value_type_id,
> + value);
> +
> +out:
> + /* end of root object */
> + jsonw_end_object(json_wtr);
> +
> + return ret;
> +}
Please add some tests for do_dump_btf(), including some invalid data type/kind.
> +
> +static struct btf *get_btf(struct bpf_map_info *map_info)
> +{
> + int btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
> + struct bpf_btf_info btf_info = { 0 };
> + __u32 len = sizeof(btf_info);
> + uint32_t last_size;
> + int err;
> + struct btf *btf = NULL;
> + void *ptr = NULL, *temp_ptr;
> +
> + if (btf_fd < 0)
> + return NULL;
> +
> + btf_info.btf_size = 4096;
Is btf_size always a constant of 4096?
> + do {
> + last_size = btf_info.btf_size;
> + temp_ptr = realloc(ptr, last_size);
> + if (!temp_ptr) {
> + p_err("unable 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;
> + }
> +
> + 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)));
> + btf = NULL;
> + }
> +
> +exit_free:
> + close(btf_fd);
> + free(ptr);
> +
> + return btf;
> +}
> +
> static int do_dump(int argc, char **argv)
> {
> void *key, *value, *prev_key;
> @@ -516,6 +597,7 @@ static int do_dump(int argc, char **argv
> __u32 len = sizeof(info);
> int err;
> int fd;
> + struct btf *btf = NULL;
>
> if (argc != 2)
> usage();
> @@ -538,6 +620,8 @@ static int do_dump(int argc, char **argv
> goto exit_free;
> }
>
> + btf = get_btf(&info);
> +
> prev_key = NULL;
> if (json_output)
> jsonw_start_array(json_wtr);
> @@ -550,9 +634,12 @@ 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);
> - else
> + if (json_output) {
> + if (btf)
> + do_dump_btf(btf, &info, key, value);
> + else
> + print_entry_json(&info, key, value);
> + } else
> print_entry_plain(&info, key, value);
> } else {
> if (json_output) {
> @@ -584,6 +671,7 @@ exit_free:
> free(key);
> free(value);
> close(fd);
> + btf__free(btf);
>
> return err;
> }
>
On Thu, Jun 21, 2018 at 12:22:52AM +0100, Song Liu wrote:
>
>
> > On Jun 20, 2018, at 1:30 PM, Okash Khawaja <[email protected]> wrote:
> >
> > This patch modifies `bpftool map dump [-j|-p] id <map-id>` to json-
> > print and pretty-json-print map dump. It calls btf_dumper introduced in
> > previous patch to accomplish this.
> >
> > The patch only prints debug info when -j or -p flags are supplied. Then
> > too, if the map has associated btf data loaded. Otherwise the usual
> > debug-less output is printed.
> >
> > Signed-off-by: Okash Khawaja <[email protected]>
> > Acked-by: Martin KaFai Lau <[email protected]>
> >
> > ---
> > tools/bpf/bpftool/map.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 91 insertions(+), 3 deletions(-)
> >
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -43,9 +43,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"
> >
> > static const char * const map_type_name[] = {
> > @@ -508,6 +512,83 @@ static int do_show(int argc, char **argv
> > return errno == ENOENT ? 0 : -1;
> > }
> >
> > +
> > +static int do_dump_btf(struct btf *btf, struct bpf_map_info *map_info,
> > + void *key, void *value)
> > +{
> > + int ret;
> > +
> > + jsonw_start_object(json_wtr);
> > + jsonw_name(json_wtr, "key");
> > +
> > + ret = btf_dumper_type(btf, json_wtr, map_info->btf_key_type_id, key);
> > + if (ret)
> > + goto out;
> > +
> > + jsonw_end_object(json_wtr);
> > +
> > + jsonw_start_object(json_wtr);
> > + jsonw_name(json_wtr, "value");
> > +
> > + ret = btf_dumper_type(btf, json_wtr, map_info->btf_value_type_id,
> > + value);
> > +
> > +out:
> > + /* end of root object */
> > + jsonw_end_object(json_wtr);
> > +
> > + return ret;
> > +}
>
> Please add some tests for do_dump_btf(), including some invalid data type/kind.
Sure. I was wondering if we can follow this up with tests afterwards.
However if they are essential now, I can add them.
>
> > +
> > +static struct btf *get_btf(struct bpf_map_info *map_info)
> > +{
> > + int btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
> > + struct bpf_btf_info btf_info = { 0 };
> > + __u32 len = sizeof(btf_info);
> > + uint32_t last_size;
> > + int err;
> > + struct btf *btf = NULL;
> > + void *ptr = NULL, *temp_ptr;
> > +
> > + if (btf_fd < 0)
> > + return NULL;
> > +
> > + btf_info.btf_size = 4096;
>
> Is btf_size always a constant of 4096?
We start out with 4096 and in the loop below we increase the size if
needed. It seems like a sane valeu to start with. However if there are
better ideas for start value, we can discuss them.
>
> > + do {
> > + last_size = btf_info.btf_size;
> > + temp_ptr = realloc(ptr, last_size);
> > + if (!temp_ptr) {
> > + p_err("unable 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;
> > + }
> > +
> > + 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)));
> > + btf = NULL;
> > + }
> > +
> > +exit_free:
> > + close(btf_fd);
> > + free(ptr);
> > +
> > + return btf;
> > +}
> > +
> > static int do_dump(int argc, char **argv)
> > {
> > void *key, *value, *prev_key;
> > @@ -516,6 +597,7 @@ static int do_dump(int argc, char **argv
> > __u32 len = sizeof(info);
> > int err;
> > int fd;
> > + struct btf *btf = NULL;
> >
> > if (argc != 2)
> > usage();
> > @@ -538,6 +620,8 @@ static int do_dump(int argc, char **argv
> > goto exit_free;
> > }
> >
> > + btf = get_btf(&info);
> > +
> > prev_key = NULL;
> > if (json_output)
> > jsonw_start_array(json_wtr);
> > @@ -550,9 +634,12 @@ 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);
> > - else
> > + if (json_output) {
> > + if (btf)
> > + do_dump_btf(btf, &info, key, value);
> > + else
> > + print_entry_json(&info, key, value);
> > + } else
> > print_entry_plain(&info, key, value);
> > } else {
> > if (json_output) {
> > @@ -584,6 +671,7 @@ exit_free:
> > free(key);
> > free(value);
> > close(fd);
> > + btf__free(btf);
> >
> > return err;
> > }
> >
>
Hi Okash,
Thanks for the patch! Please find some nitpicks inline below.
2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <[email protected]>
> This patch modifies `bpftool map dump [-j|-p] id <map-id>` to json-
> print and pretty-json-print map dump. It calls btf_dumper introduced in
> previous patch to accomplish this.
>
> The patch only prints debug info when -j or -p flags are supplied. Then
> too, if the map has associated btf data loaded. Otherwise the usual
> debug-less output is printed.
>
> Signed-off-by: Okash Khawaja <[email protected]>
> Acked-by: Martin KaFai Lau <[email protected]>
>
> ---
> tools/bpf/bpftool/map.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -43,9 +43,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"
>
> static const char * const map_type_name[] = {
> @@ -508,6 +512,83 @@ static int do_show(int argc, char **argv
> return errno == ENOENT ? 0 : -1;
> }
>
> +
> +static int do_dump_btf(struct btf *btf, struct bpf_map_info *map_info,
> + void *key, void *value)
Nit: Please align the second line on the opening parenthesis.
> +{
> + int ret;
> +
> + jsonw_start_object(json_wtr);
> + jsonw_name(json_wtr, "key");
> +
> + ret = btf_dumper_type(btf, json_wtr, map_info->btf_key_type_id, key);
> + if (ret)
> + goto out;
> +
> + jsonw_end_object(json_wtr);
> +
> + jsonw_start_object(json_wtr);
> + jsonw_name(json_wtr, "value");
> +
> + ret = btf_dumper_type(btf, json_wtr, map_info->btf_value_type_id,
> + value);
Same comment.
> +
> +out:
> + /* end of root object */
> + jsonw_end_object(json_wtr);
This is not the root JSON object, which is not produced in that
function, so I find the comment misleading.
I also find it confusing that it closes the first JSON object of this
function if there is an error, but the second if "btf_dumper_type()"
succeeds. What about the following: closing the first object in all
cases, before evaluating the value of "ret", and if "ret" is non-null
returning immediately; and completely removing the "goto" from this
function?
> +
> + 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);
> + struct bpf_btf_info btf_info = { 0 };
> + __u32 len = sizeof(btf_info);
> + uint32_t last_size;
> + int err;
> + struct btf *btf = NULL;
> + void *ptr = NULL, *temp_ptr;
Nit: please sort declarations in reverse-Christmas-tree order.
> +
> + if (btf_fd < 0)
> + return NULL;
> +
> + btf_info.btf_size = 4096;
> + do {
> + last_size = btf_info.btf_size;
> + temp_ptr = realloc(ptr, last_size);
> + if (!temp_ptr) {
> + p_err("unable allocate memory for debug info.");
"unable *to* allocate"?
(Also most other error messages do not end with a period, but here this
is just me being fussy.)
> + 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 I understand correctly, the first time you try to retrieve up to 4096
bytes, and if the btf_info is larger than this, you try a second time
with the size returned in btf_info.btf_size instead. I don't find it
intuitive (but maybe this is just me?), do you think you could add a
comment above this bloc maybe?
> +
> + 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");
Nit: Please align the second line on the opening parenthesis.
> + goto exit_free;
> + }
> +
> + btf = btf__new((uint8_t *) btf_info.btf,
Nit: No space between the cast and the name of the variable.
> + btf_info.btf_size, NULL);
Same remark on parenthesis here...
> + if (IS_ERR(btf)) {
> + printf("error when initialising btf: %s\n",
> + strerror(PTR_ERR(btf)));
... and here.
> + btf = NULL;
> + }
> +
> +exit_free:
> + close(btf_fd);
> + free(ptr);
> +
> + return btf;
> +}
> +
> static int do_dump(int argc, char **argv)
> {
> void *key, *value, *prev_key;
> @@ -516,6 +597,7 @@ static int do_dump(int argc, char **argv
> __u32 len = sizeof(info);
> int err;
> int fd;
> + struct btf *btf = NULL;
Reverse-Christmas-tree order, please.
>
> if (argc != 2)
> usage();
> @@ -538,6 +620,8 @@ static int do_dump(int argc, char **argv
> goto exit_free;
> }
>
> + btf = get_btf(&info);
> +
> prev_key = NULL;
> if (json_output)
> jsonw_start_array(json_wtr);
> @@ -550,9 +634,12 @@ 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);
> - else
> + if (json_output) {
> + if (btf)
> + do_dump_btf(btf, &info, key, value);
> + else
> + print_entry_json(&info, key, value);
> + } else
> print_entry_plain(&info, key, value);
Please add brackets around "print_entry_plain()" (to harmonise with the
"if" of the same bloc).
> } else {
> if (json_output) {
> @@ -584,6 +671,7 @@ exit_free:
> free(key);
> free(value);
> close(fd);
> + btf__free(btf);
>
> return err;
> }
>
Thanks,
Quentin
Hi Quentin,
On Thu, Jun 21, 2018 at 11:24:59AM +0100, Quentin Monnet wrote:
> Hi Okash,
>
> Thanks for the patch! Please find some nitpicks inline below.
Thanks for your feedback. All of it makes sense so I'll send v2 with
those changes. Couple of responses are inlined below.
>
> 2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <[email protected]>
> > This patch modifies `bpftool map dump [-j|-p] id <map-id>` to json-
> > print and pretty-json-print map dump. It calls btf_dumper introduced in
> > previous patch to accomplish this.
> >
> > The patch only prints debug info when -j or -p flags are supplied. Then
> > too, if the map has associated btf data loaded. Otherwise the usual
> > debug-less output is printed.
> >
> > Signed-off-by: Okash Khawaja <[email protected]>
> > Acked-by: Martin KaFai Lau <[email protected]>
> >
> > ---
> > tools/bpf/bpftool/map.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 91 insertions(+), 3 deletions(-)
> >
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -43,9 +43,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"
> >
> > static const char * const map_type_name[] = {
> > @@ -508,6 +512,83 @@ static int do_show(int argc, char **argv
> > return errno == ENOENT ? 0 : -1;
> > }
> >
> > +
> > +static int do_dump_btf(struct btf *btf, struct bpf_map_info *map_info,
> > + void *key, void *value)
>
> Nit: Please align the second line on the opening parenthesis.
>
> > +{
> > + int ret;
> > +
> > + jsonw_start_object(json_wtr);
> > + jsonw_name(json_wtr, "key");
> > +
> > + ret = btf_dumper_type(btf, json_wtr, map_info->btf_key_type_id, key);
> > + if (ret)
> > + goto out;
> > +
> > + jsonw_end_object(json_wtr);
> > +
> > + jsonw_start_object(json_wtr);
> > + jsonw_name(json_wtr, "value");
> > +
> > + ret = btf_dumper_type(btf, json_wtr, map_info->btf_value_type_id,
> > + value);
>
> Same comment.
>
> > +
> > +out:
> > + /* end of root object */
> > + jsonw_end_object(json_wtr);
>
> This is not the root JSON object, which is not produced in that
> function, so I find the comment misleading.
>
> I also find it confusing that it closes the first JSON object of this
> function if there is an error, but the second if "btf_dumper_type()"
> succeeds. What about the following: closing the first object in all
> cases, before evaluating the value of "ret", and if "ret" is non-null
> returning immediately; and completely removing the "goto" from this
> function?
Code will be more intuitive that way so I'll re-organise it accordingly.
>
> > +
> > + 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);
> > + struct bpf_btf_info btf_info = { 0 };
> > + __u32 len = sizeof(btf_info);
> > + uint32_t last_size;
> > + int err;
> > + struct btf *btf = NULL;
> > + void *ptr = NULL, *temp_ptr;
>
> Nit: please sort declarations in reverse-Christmas-tree order.
>
> > +
> > + if (btf_fd < 0)
> > + return NULL;
> > +
> > + btf_info.btf_size = 4096;
> > + do {
> > + last_size = btf_info.btf_size;
> > + temp_ptr = realloc(ptr, last_size);
> > + if (!temp_ptr) {
> > + p_err("unable allocate memory for debug info.");
>
> "unable *to* allocate"?
> (Also most other error messages do not end with a period, but here this
> is just me being fussy.)
I think it makes sense to be consistent. I'll remove the full stop.
>
> > + 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 I understand correctly, the first time you try to retrieve up to 4096
> bytes, and if the btf_info is larger than this, you try a second time
> with the size returned in btf_info.btf_size instead. I don't find it
> intuitive (but maybe this is just me?), do you think you could add a
> comment above this bloc maybe?
Yes that is what this code is doing. I'll add comments explaining it.
>
> > +
> > + 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");
>
> Nit: Please align the second line on the opening parenthesis.
>
> > + goto exit_free;
> > + }
> > +
> > + btf = btf__new((uint8_t *) btf_info.btf,
>
> Nit: No space between the cast and the name of the variable.
>
> > + btf_info.btf_size, NULL);
>
> Same remark on parenthesis here...
>
> > + if (IS_ERR(btf)) {
> > + printf("error when initialising btf: %s\n",
> > + strerror(PTR_ERR(btf)));
>
> ... and here.
>
> > + btf = NULL;
> > + }
> > +
> > +exit_free:
> > + close(btf_fd);
> > + free(ptr);
> > +
> > + return btf;
> > +}
> > +
> > static int do_dump(int argc, char **argv)
> > {
> > void *key, *value, *prev_key;
> > @@ -516,6 +597,7 @@ static int do_dump(int argc, char **argv
> > __u32 len = sizeof(info);
> > int err;
> > int fd;
> > + struct btf *btf = NULL;
>
> Reverse-Christmas-tree order, please.
>
> >
> > if (argc != 2)
> > usage();
> > @@ -538,6 +620,8 @@ static int do_dump(int argc, char **argv
> > goto exit_free;
> > }
> >
> > + btf = get_btf(&info);
> > +
> > prev_key = NULL;
> > if (json_output)
> > jsonw_start_array(json_wtr);
> > @@ -550,9 +634,12 @@ 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);
> > - else
> > + if (json_output) {
> > + if (btf)
> > + do_dump_btf(btf, &info, key, value);
> > + else
> > + print_entry_json(&info, key, value);
> > + } else
> > print_entry_plain(&info, key, value);
>
> Please add brackets around "print_entry_plain()" (to harmonise with the
> "if" of the same bloc).
>
> > } else {
> > if (json_output) {
> > @@ -584,6 +671,7 @@ exit_free:
> > free(key);
> > free(value);
> > close(fd);
> > + btf__free(btf);
> >
> > return err;
> > }
> >
>
> Thanks,
> Quentin