| Message ID | 20190523204222.3998365-11-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BTF-to-C converter | expand |
On Thu, 23 May 2019 13:42:20 -0700, Andrii Nakryiko wrote: > Utilize new libbpf's btf_dump API to emit BTF as a C definitions. > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com> Thanks!
Hi Andrii, Some nits inline, nothing blocking though. 2019-05-23 13:42 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com> > Utilize new libbpf's btf_dump API to emit BTF as a C definitions. > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > tools/bpf/bpftool/btf.c | 74 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 72 insertions(+), 2 deletions(-) > > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c > index a22ef6587ebe..1cdbfad42b38 100644 > --- a/tools/bpf/bpftool/btf.c > +++ b/tools/bpf/bpftool/btf.c > @@ -340,11 +340,48 @@ static int dump_btf_raw(const struct btf *btf, > return 0; > } > > +static void btf_dump_printf(void *ctx, const char *fmt, va_list args) Nit: This function could have a printf attribute ("__printf(2, 0)"). > +{ > + vfprintf(stdout, fmt, args); > +} > + > +static int dump_btf_c(const struct btf *btf, > + __u32 *root_type_ids, int root_type_cnt) > +{ > + struct btf_dump *d; > + int err = 0, i; > + > + d = btf_dump__new(btf, NULL, NULL, btf_dump_printf); > + if (IS_ERR(d)) > + return PTR_ERR(d); > + > + if (root_type_cnt) { > + for (i = 0; i < root_type_cnt; i++) { > + err = btf_dump__dump_type(d, root_type_ids[i]); > + if (err) > + goto done; > + } > + } else { > + int cnt = btf__get_nr_types(btf); > + > + for (i = 1; i <= cnt; i++) { > + err = btf_dump__dump_type(d, i); > + if (err) > + goto done; > + } > + } > + > +done: > + btf_dump__free(d); > + return err; > +} > + > static int do_dump(int argc, char **argv) > { > struct btf *btf = NULL; > __u32 root_type_ids[2]; > int root_type_cnt = 0; > + bool dump_c = false; > __u32 btf_id = -1; > const char *src; > int fd = -1; > @@ -431,6 +468,29 @@ static int do_dump(int argc, char **argv) > goto done; > } > > + while (argc) { > + if (is_prefix(*argv, "format")) { > + NEXT_ARG(); > + if (argc < 1) { > + p_err("expecting value for 'format' option\n"); > + goto done; > + } > + if (strcmp(*argv, "c") == 0) { > + dump_c = true; > + } else if (strcmp(*argv, "raw") == 0) { Do you think we could use is_prefix() instead of strcmp() here? > + dump_c = false; > + } else { > + p_err("unrecognized format specifier: '%s'", > + *argv); Would it be worth reminding the user about the valid specifiers in that message? (But then we already have it in do_help(), so maybe not.) > + goto done; > + } > + NEXT_ARG(); > + } else { > + p_err("unrecognized option: '%s'", *argv); > + goto done; > + } > + } > + > if (!btf) { > err = btf__get_from_id(btf_id, &btf); > if (err) { > @@ -444,7 +504,16 @@ static int do_dump(int argc, char **argv) > } > } > > - dump_btf_raw(btf, root_type_ids, root_type_cnt); > + if (dump_c) { > + if (json_output) { > + p_err("JSON output for C-syntax dump is not supported"); > + err = -ENOTSUP; > + goto done; > + } > + err = dump_btf_c(btf, root_type_ids, root_type_cnt); > + } else { > + err = dump_btf_raw(btf, root_type_ids, root_type_cnt); > + } > > done: > close(fd); > @@ -460,10 +529,11 @@ static int do_help(int argc, char **argv) > } > > fprintf(stderr, > - "Usage: %s btf dump BTF_SRC\n" > + "Usage: %s btf dump BTF_SRC [format FORMAT]\n" > " %s btf help\n" > "\n" > " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n" > + " FORMAT := { raw | c }\n" > " " HELP_SPEC_MAP "\n" > " " HELP_SPEC_PROGRAM "\n" > " " HELP_SPEC_OPTIONS "\n" >
On Fri, May 24, 2019 at 2:14 AM Quentin Monnet <quentin.monnet@netronome.com> wrote: > > Hi Andrii, > > Some nits inline, nothing blocking though. > > 2019-05-23 13:42 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com> > > Utilize new libbpf's btf_dump API to emit BTF as a C definitions. > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > tools/bpf/bpftool/btf.c | 74 +++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 72 insertions(+), 2 deletions(-) > > > > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c > > index a22ef6587ebe..1cdbfad42b38 100644 > > --- a/tools/bpf/bpftool/btf.c > > +++ b/tools/bpf/bpftool/btf.c > > @@ -340,11 +340,48 @@ static int dump_btf_raw(const struct btf *btf, > > return 0; > > } > > > > +static void btf_dump_printf(void *ctx, const char *fmt, va_list args) > > Nit: This function could have a printf attribute ("__printf(2, 0)"). added, though I don't think it matters as it's only used as a callback function. > > > +{ > > + vfprintf(stdout, fmt, args); > > +} > > + > > +static int dump_btf_c(const struct btf *btf, > > + __u32 *root_type_ids, int root_type_cnt) > > +{ > > + struct btf_dump *d; > > + int err = 0, i; > > + > > + d = btf_dump__new(btf, NULL, NULL, btf_dump_printf); > > + if (IS_ERR(d)) > > + return PTR_ERR(d); > > + > > + if (root_type_cnt) { > > + for (i = 0; i < root_type_cnt; i++) { > > + err = btf_dump__dump_type(d, root_type_ids[i]); > > + if (err) > > + goto done; > > + } > > + } else { > > + int cnt = btf__get_nr_types(btf); > > + > > + for (i = 1; i <= cnt; i++) { > > + err = btf_dump__dump_type(d, i); > > + if (err) > > + goto done; > > + } > > + } > > + > > +done: > > + btf_dump__free(d); > > + return err; > > +} > > + > > static int do_dump(int argc, char **argv) > > { > > struct btf *btf = NULL; > > __u32 root_type_ids[2]; > > int root_type_cnt = 0; > > + bool dump_c = false; > > __u32 btf_id = -1; > > const char *src; > > int fd = -1; > > @@ -431,6 +468,29 @@ static int do_dump(int argc, char **argv) > > goto done; > > } > > > > + while (argc) { > > + if (is_prefix(*argv, "format")) { > > + NEXT_ARG(); > > + if (argc < 1) { > > + p_err("expecting value for 'format' option\n"); > > + goto done; > > + } > > + if (strcmp(*argv, "c") == 0) { > > + dump_c = true; > > + } else if (strcmp(*argv, "raw") == 0) { > > Do you think we could use is_prefix() instead of strcmp() here? So I considered it, and then decided against it, though I can still be convinced otherwise. Right now we have raw and c, but let's say we add rust as an option. r will become ambiguous, but actually will be resolved to whatever we check first: either raw or rust, which is not great. So given that those format specifiers will tend to be short, I decided it's ok to require to specify them fully. Does it make sense? > > > + dump_c = false; > > + } else { > > + p_err("unrecognized format specifier: '%s'", > > + *argv); > > Would it be worth reminding the user about the valid specifiers in that > message? (But then we already have it in do_help(), so maybe not.) Added possible options to the message. > > > + goto done; > > + } > > + NEXT_ARG(); > > + } else { > > + p_err("unrecognized option: '%s'", *argv); > > + goto done; > > + } > > + } > > + > > if (!btf) { > > err = btf__get_from_id(btf_id, &btf); > > if (err) { > > @@ -444,7 +504,16 @@ static int do_dump(int argc, char **argv) > > } > > } > > > > - dump_btf_raw(btf, root_type_ids, root_type_cnt); > > + if (dump_c) { > > + if (json_output) { > > + p_err("JSON output for C-syntax dump is not supported"); > > + err = -ENOTSUP; > > + goto done; > > + } > > + err = dump_btf_c(btf, root_type_ids, root_type_cnt); > > + } else { > > + err = dump_btf_raw(btf, root_type_ids, root_type_cnt); > > + } > > > > done: > > close(fd); > > @@ -460,10 +529,11 @@ static int do_help(int argc, char **argv) > > } > > > > fprintf(stderr, > > - "Usage: %s btf dump BTF_SRC\n" > > + "Usage: %s btf dump BTF_SRC [format FORMAT]\n" > > " %s btf help\n" > > "\n" > > " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n" > > + " FORMAT := { raw | c }\n" > > " " HELP_SPEC_MAP "\n" > > " " HELP_SPEC_PROGRAM "\n" > > " " HELP_SPEC_OPTIONS "\n" > > >
2019-05-24 10:14 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Fri, May 24, 2019 at 2:14 AM Quentin Monnet > <quentin.monnet@netronome.com> wrote: >> >> Hi Andrii, >> >> Some nits inline, nothing blocking though. >> >> 2019-05-23 13:42 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com> >>> Utilize new libbpf's btf_dump API to emit BTF as a C definitions. >>> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> >>> --- >>> tools/bpf/bpftool/btf.c | 74 +++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 72 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c >>> index a22ef6587ebe..1cdbfad42b38 100644 >>> --- a/tools/bpf/bpftool/btf.c >>> +++ b/tools/bpf/bpftool/btf.c >>> @@ -340,11 +340,48 @@ static int dump_btf_raw(const struct btf *btf, >>> return 0; >>> } >>> >>> +static void btf_dump_printf(void *ctx, const char *fmt, va_list args) >> >> Nit: This function could have a printf attribute ("__printf(2, 0)"). > > added, though I don't think it matters as it's only used as a callback function. Thanks. Yes, true... But the attribute does not hurt, and we have it case it changes in the future and the function is reused. Ok, unlikely, but... > >> >>> +{ >>> + vfprintf(stdout, fmt, args); >>> +} >>> + >>> @@ -431,6 +468,29 @@ static int do_dump(int argc, char **argv) >>> goto done; >>> } >>> >>> + while (argc) { >>> + if (is_prefix(*argv, "format")) { >>> + NEXT_ARG(); >>> + if (argc < 1) { >>> + p_err("expecting value for 'format' option\n"); >>> + goto done; >>> + } >>> + if (strcmp(*argv, "c") == 0) { >>> + dump_c = true; >>> + } else if (strcmp(*argv, "raw") == 0) { >> >> Do you think we could use is_prefix() instead of strcmp() here? > > So I considered it, and then decided against it, though I can still be > convinced otherwise. Right now we have raw and c, but let's say we add > rust as an option. r will become ambiguous, but actually will be > resolved to whatever we check first: either raw or rust, which is not > great. So given that those format specifiers will tend to be short, I > decided it's ok to require to specify them fully. Does it make sense? It does make sense. I thought about that too. I think I would add prefix handling anyway, especially because "raw" is the default so it makes sense defaulting to it in case there is a collision in the future. This is what happens already between "bpftool prog" and "bpftool perf". But I don't really mind, so ok, let's keep the full keyword for now if you prefer. > >> >>> + dump_c = false; >>> + } else { >>> + p_err("unrecognized format specifier: '%s'", >>> + *argv); >> >> Would it be worth reminding the user about the valid specifiers in that >> message? (But then we already have it in do_help(), so maybe not.) > > Added possible options to the message. Cool, thanks!
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c index a22ef6587ebe..1cdbfad42b38 100644 --- a/tools/bpf/bpftool/btf.c +++ b/tools/bpf/bpftool/btf.c @@ -340,11 +340,48 @@ static int dump_btf_raw(const struct btf *btf, return 0; } +static void btf_dump_printf(void *ctx, const char *fmt, va_list args) +{ + vfprintf(stdout, fmt, args); +} + +static int dump_btf_c(const struct btf *btf, + __u32 *root_type_ids, int root_type_cnt) +{ + struct btf_dump *d; + int err = 0, i; + + d = btf_dump__new(btf, NULL, NULL, btf_dump_printf); + if (IS_ERR(d)) + return PTR_ERR(d); + + if (root_type_cnt) { + for (i = 0; i < root_type_cnt; i++) { + err = btf_dump__dump_type(d, root_type_ids[i]); + if (err) + goto done; + } + } else { + int cnt = btf__get_nr_types(btf); + + for (i = 1; i <= cnt; i++) { + err = btf_dump__dump_type(d, i); + if (err) + goto done; + } + } + +done: + btf_dump__free(d); + return err; +} + static int do_dump(int argc, char **argv) { struct btf *btf = NULL; __u32 root_type_ids[2]; int root_type_cnt = 0; + bool dump_c = false; __u32 btf_id = -1; const char *src; int fd = -1; @@ -431,6 +468,29 @@ static int do_dump(int argc, char **argv) goto done; } + while (argc) { + if (is_prefix(*argv, "format")) { + NEXT_ARG(); + if (argc < 1) { + p_err("expecting value for 'format' option\n"); + goto done; + } + if (strcmp(*argv, "c") == 0) { + dump_c = true; + } else if (strcmp(*argv, "raw") == 0) { + dump_c = false; + } else { + p_err("unrecognized format specifier: '%s'", + *argv); + goto done; + } + NEXT_ARG(); + } else { + p_err("unrecognized option: '%s'", *argv); + goto done; + } + } + if (!btf) { err = btf__get_from_id(btf_id, &btf); if (err) { @@ -444,7 +504,16 @@ static int do_dump(int argc, char **argv) } } - dump_btf_raw(btf, root_type_ids, root_type_cnt); + if (dump_c) { + if (json_output) { + p_err("JSON output for C-syntax dump is not supported"); + err = -ENOTSUP; + goto done; + } + err = dump_btf_c(btf, root_type_ids, root_type_cnt); + } else { + err = dump_btf_raw(btf, root_type_ids, root_type_cnt); + } done: close(fd); @@ -460,10 +529,11 @@ static int do_help(int argc, char **argv) } fprintf(stderr, - "Usage: %s btf dump BTF_SRC\n" + "Usage: %s btf dump BTF_SRC [format FORMAT]\n" " %s btf help\n" "\n" " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n" + " FORMAT := { raw | c }\n" " " HELP_SPEC_MAP "\n" " " HELP_SPEC_PROGRAM "\n" " " HELP_SPEC_OPTIONS "\n"
Utilize new libbpf's btf_dump API to emit BTF as a C definitions. Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/bpf/bpftool/btf.c | 74 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 2 deletions(-)