| Message ID | 20190522195053.4017624-11-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BTF-to-C converter | expand |
On Wed, 22 May 2019 12:50:51 -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> > --- > tools/bpf/bpftool/btf.c | 63 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 60 insertions(+), 3 deletions(-) > > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c > index a22ef6587ebe..ed3d3221cc78 100644 > --- a/tools/bpf/bpftool/btf.c > +++ b/tools/bpf/bpftool/btf.c > @@ -1,5 +1,12 @@ > // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > -/* Copyright (C) 2019 Facebook */ > + > +/* > + * BTF dumping command. > + * Load BTF from multiple possible sources and outptu entirety or subset of > + * types in either raw format or C-syntax format. > + * I don't think this header adds any value. Its very unlikely people will remember to update it. And it's misspelled to begin with. Please remove. > + * Copyright (C) 2019 Facebook > + */ > > #include <errno.h> > #include <fcntl.h> > @@ -340,11 +347,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) Please break the line after static int. > +{ > + struct btf_dump *d; > + int err = 0, i, id; Hmm.. why do you have both i and id here? Maybe my eyes are failing me but it seems either one or the other is used in different branches of the main if () :) > + 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 (id = 1; id <= cnt; id++) { > + err = btf_dump__dump_type(d, id); > + if (err) > + goto done; > + } > + } > + > +done: > + btf_dump__free(d); > + return err; What do we do for JSON output? > +} > + > 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 +475,16 @@ static int do_dump(int argc, char **argv) > goto done; > } > > + while (argc) { > + if (strcmp(*argv, "c") == 0) { > + dump_c = true; > + NEXT_ARG(); > + } else { > + p_err("unrecognized option: '%s'", *argv); > + goto done; > + } > + } This code should have checked there are no arguments and return an error from the start :S > if (!btf) { > err = btf__get_from_id(btf_id, &btf); > if (err) { > @@ -444,7 +498,10 @@ static int do_dump(int argc, char **argv) > } > } > > - dump_btf_raw(btf, root_type_ids, root_type_cnt); > + if (dump_c) > + dump_btf_c(btf, root_type_ids, root_type_cnt); > + else > + dump_btf_raw(btf, root_type_ids, root_type_cnt); > > done: > close(fd); > @@ -460,7 +517,7 @@ static int do_help(int argc, char **argv) > } > > fprintf(stderr, > - "Usage: %s btf dump BTF_SRC\n" > + "Usage: %s btf dump BTF_SRC [c]\n" bpftool generally uses <key value> formats. So perhaps we could do something like "[format raw|c]" here for consistency, defaulting to raw? > " %s btf help\n" > "\n" > " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n"
On Wed, May 22, 2019 at 5:25 PM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 22 May 2019 12:50:51 -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> > > --- > > tools/bpf/bpftool/btf.c | 63 +++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 60 insertions(+), 3 deletions(-) > > > > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c > > index a22ef6587ebe..ed3d3221cc78 100644 > > --- a/tools/bpf/bpftool/btf.c > > +++ b/tools/bpf/bpftool/btf.c > > @@ -1,5 +1,12 @@ > > // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > -/* Copyright (C) 2019 Facebook */ > > + > > +/* > > + * BTF dumping command. > > + * Load BTF from multiple possible sources and outptu entirety or subset of > > + * types in either raw format or C-syntax format. > > + * > > I don't think this header adds any value. Its very unlikely people > will remember to update it. And it's misspelled to begin with. > Please remove. OK, will remove. > > > + * Copyright (C) 2019 Facebook > > + */ > > > > #include <errno.h> > > #include <fcntl.h> > > @@ -340,11 +347,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) > > Please break the line after static int. I don't mind, but it seems that prevalent formatting for such cases (at least in bpftool code base) is aligning arguments and not break static <return type> into separate line: // multi-line function definitions with static on the same line $ rg '^static \w+.*\([^\)]*$' | wc -l 45 // multi-line function definitions with static on separate line $ rg '^static \w+[^\(\{;]*$' | wc -l 12 So I don't mind changing, but which one is canonical way of formatting? > > > +{ > > + struct btf_dump *d; > > + int err = 0, i, id; > > Hmm.. why do you have both i and id here? Maybe my eyes are failing me > but it seems either one or the other is used in different branches of > the main if () :) You are right. i is used as an index into array of IDs, while for the other branch we iterate type IDs explicitly. I thought it's less confusing, but apparently it's the other way. I can do everything with just 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 (id = 1; id <= cnt; id++) { > > + err = btf_dump__dump_type(d, id); > > + if (err) > > + goto done; > > + } > > + } > > + > > +done: > > + btf_dump__free(d); > > + return err; > > What do we do for JSON output? Still dump C syntax. What do you propose? Error out if json enabled? > > > +} > > + > > 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 +475,16 @@ static int do_dump(int argc, char **argv) > > goto done; > > } > > > > + while (argc) { > > + if (strcmp(*argv, "c") == 0) { > > + dump_c = true; > > + NEXT_ARG(); > > + } else { > > + p_err("unrecognized option: '%s'", *argv); > > + goto done; > > + } > > + } > > This code should have checked there are no arguments and return an > error from the start :S I might be missing your point here. Lack of extra options is not an error, they are optional. It's just if there is an option, that we can't recognize - then we error out. > > > if (!btf) { > > err = btf__get_from_id(btf_id, &btf); > > if (err) { > > @@ -444,7 +498,10 @@ static int do_dump(int argc, char **argv) > > } > > } > > > > - dump_btf_raw(btf, root_type_ids, root_type_cnt); > > + if (dump_c) > > + dump_btf_c(btf, root_type_ids, root_type_cnt); > > + else > > + dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > > done: > > close(fd); > > @@ -460,7 +517,7 @@ static int do_help(int argc, char **argv) > > } > > > > fprintf(stderr, > > - "Usage: %s btf dump BTF_SRC\n" > > + "Usage: %s btf dump BTF_SRC [c]\n" > > bpftool generally uses <key value> formats. So perhaps we could do > something like "[format raw|c]" here for consistency, defaulting to raw? That's not true for options, though. I see that at cgroup, prog, and some map subcommands (haven't checked all other) just accept a list of options without extra identifying key. > > > " %s btf help\n" > > "\n" > > " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n" >
On Wed, 22 May 2019 17:58:23 -0700, Andrii Nakryiko wrote: > On Wed, May 22, 2019 at 5:25 PM Jakub Kicinski wrote: > > On Wed, 22 May 2019 12:50:51 -0700, Andrii Nakryiko wrote: > > > + * Copyright (C) 2019 Facebook > > > + */ > > > > > > #include <errno.h> > > > #include <fcntl.h> > > > @@ -340,11 +347,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) > > > > Please break the line after static int. > > I don't mind, but it seems that prevalent formatting for such cases > (at least in bpftool code base) is aligning arguments and not break > static <return type> into separate line: > > // multi-line function definitions with static on the same line > $ rg '^static \w+.*\([^\)]*$' | wc -l > 45 > // multi-line function definitions with static on separate line > $ rg '^static \w+[^\(\{;]*$' | wc -l > 12 > > So I don't mind changing, but which one is canonical way of formatting? Not really, just my preference :) In my experience having the return type on a separate line if its longer than a few chars is the simplest rule for consistent and good looking code. > > > + 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 (id = 1; id <= cnt; id++) { > > > + err = btf_dump__dump_type(d, id); > > > + if (err) > > > + goto done; > > > + } > > > + } > > > + > > > +done: > > > + btf_dump__free(d); > > > + return err; > > > > What do we do for JSON output? > > Still dump C syntax. What do you propose? Error out if json enabled? I wonder. Letting it just print C is going to confuse anything that just feeds the output into a JSON parser. I'd err on the side of returning an error, we can always relax that later if we find a use case of returning C syntax via JSON. > > > +} > > > + > > > 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 +475,16 @@ static int do_dump(int argc, char **argv) > > > goto done; > > > } > > > > > > + while (argc) { > > > + if (strcmp(*argv, "c") == 0) { > > > + dump_c = true; > > > + NEXT_ARG(); > > > + } else { > > > + p_err("unrecognized option: '%s'", *argv); > > > + goto done; > > > + } > > > + } > > > > This code should have checked there are no arguments and return an > > error from the start :S > > I might be missing your point here. Lack of extra options is not an > error, they are optional. It's just if there is an option, that we > can't recognize - then we error out. Oh, I was just remarking that before your patch bpftool would not error if garbage options were passed, it'd be better if we errored from the start. But too late now, your code is good 👍 > > > if (!btf) { > > > err = btf__get_from_id(btf_id, &btf); > > > if (err) { > > > @@ -444,7 +498,10 @@ static int do_dump(int argc, char **argv) > > > } > > > } > > > > > > - dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > + if (dump_c) > > > + dump_btf_c(btf, root_type_ids, root_type_cnt); > > > + else > > > + dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > > > > done: > > > close(fd); > > > @@ -460,7 +517,7 @@ static int do_help(int argc, char **argv) > > > } > > > > > > fprintf(stderr, > > > - "Usage: %s btf dump BTF_SRC\n" > > > + "Usage: %s btf dump BTF_SRC [c]\n" > > > > bpftool generally uses <key value> formats. So perhaps we could do > > something like "[format raw|c]" here for consistency, defaulting to raw? > > That's not true for options, though. I see that at cgroup, prog, and > some map subcommands (haven't checked all other) just accept a list of > options without extra identifying key. Yeah, we weren't 100% enforcing this rule and it's a bit messy now :/ > > > " %s btf help\n" > > > "\n" > > > " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n"
On Wed, May 22, 2019 at 6:23 PM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 22 May 2019 17:58:23 -0700, Andrii Nakryiko wrote: > > On Wed, May 22, 2019 at 5:25 PM Jakub Kicinski wrote: > > > On Wed, 22 May 2019 12:50:51 -0700, Andrii Nakryiko wrote: > > > > + * Copyright (C) 2019 Facebook > > > > + */ > > > > > > > > #include <errno.h> > > > > #include <fcntl.h> > > > > @@ -340,11 +347,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) > > > > > > Please break the line after static int. > > > > I don't mind, but it seems that prevalent formatting for such cases > > (at least in bpftool code base) is aligning arguments and not break > > static <return type> into separate line: > > > > // multi-line function definitions with static on the same line > > $ rg '^static \w+.*\([^\)]*$' | wc -l > > 45 > > // multi-line function definitions with static on separate line > > $ rg '^static \w+[^\(\{;]*$' | wc -l > > 12 > > > > So I don't mind changing, but which one is canonical way of formatting? > > Not really, just my preference :) I'll stick to majority :) I feel like it's also a preferred style in libbpf, so I'd rather converge to that. > > In my experience having the return type on a separate line if its > longer than a few chars is the simplest rule for consistent and good > looking code. > > > > > + 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 (id = 1; id <= cnt; id++) { > > > > + err = btf_dump__dump_type(d, id); > > > > + if (err) > > > > + goto done; > > > > + } > > > > + } > > > > + > > > > +done: > > > > + btf_dump__free(d); > > > > + return err; > > > > > > What do we do for JSON output? > > > > Still dump C syntax. What do you propose? Error out if json enabled? > > I wonder. Letting it just print C is going to confuse anything that > just feeds the output into a JSON parser. I'd err on the side of > returning an error, we can always relax that later if we find a use > case of returning C syntax via JSON. Ok, I'll emit error (seems like pr_err automatically handles JSON output, which is very nice). > > > > > +} > > > > + > > > > 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 +475,16 @@ static int do_dump(int argc, char **argv) > > > > goto done; > > > > } > > > > > > > > + while (argc) { > > > > + if (strcmp(*argv, "c") == 0) { > > > > + dump_c = true; > > > > + NEXT_ARG(); > > > > + } else { > > > > + p_err("unrecognized option: '%s'", *argv); > > > > + goto done; > > > > + } > > > > + } > > > > > > This code should have checked there are no arguments and return an > > > error from the start :S > > > > I might be missing your point here. Lack of extra options is not an > > error, they are optional. It's just if there is an option, that we > > can't recognize - then we error out. > > Oh, I was just remarking that before your patch bpftool would not error > if garbage options were passed, it'd be better if we errored from the > start. But too late now, your code is good > Ah, I see, yeah, that was my original omission, you are right. > > > > if (!btf) { > > > > err = btf__get_from_id(btf_id, &btf); > > > > if (err) { > > > > @@ -444,7 +498,10 @@ static int do_dump(int argc, char **argv) > > > > } > > > > } > > > > > > > > - dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > > + if (dump_c) > > > > + dump_btf_c(btf, root_type_ids, root_type_cnt); > > > > + else > > > > + dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > > > > > > done: > > > > close(fd); > > > > @@ -460,7 +517,7 @@ static int do_help(int argc, char **argv) > > > > } > > > > > > > > fprintf(stderr, > > > > - "Usage: %s btf dump BTF_SRC\n" > > > > + "Usage: %s btf dump BTF_SRC [c]\n" > > > > > > bpftool generally uses <key value> formats. So perhaps we could do > > > something like "[format raw|c]" here for consistency, defaulting to raw? > > > > That's not true for options, though. I see that at cgroup, prog, and > > some map subcommands (haven't checked all other) just accept a list of > > options without extra identifying key. > > Yeah, we weren't 100% enforcing this rule and it's a bit messy now :/ Unless you feel very strongly about this, it seems ok to me to allow "boolean options" (similarly to boolean --flag args) as a stand-alone set of tags. bpftool invocations are already very verbose, no need to add to that. Plus it also makes bash-completion simpler, it's always good not to complicate bash script unnecessarily :) > > > > > " %s btf help\n" > > > > "\n" > > > > " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\n"
On Wed, 22 May 2019 21:43:43 -0700, Andrii Nakryiko wrote: > On Wed, May 22, 2019 at 6:23 PM Jakub Kicinski wrote: > > On Wed, 22 May 2019 17:58:23 -0700, Andrii Nakryiko wrote: > > > On Wed, May 22, 2019 at 5:25 PM Jakub Kicinski wrote: > > > > On Wed, 22 May 2019 12:50:51 -0700, Andrii Nakryiko wrote: > > > > > + * Copyright (C) 2019 Facebook > > > > > + */ > > > > > > > > > > #include <errno.h> > > > > > #include <fcntl.h> > > > > > @@ -340,11 +347,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) > > > > > > > > Please break the line after static int. > > > > > > I don't mind, but it seems that prevalent formatting for such cases > > > (at least in bpftool code base) is aligning arguments and not break > > > static <return type> into separate line: > > > > > > // multi-line function definitions with static on the same line > > > $ rg '^static \w+.*\([^\)]*$' | wc -l > > > 45 > > > // multi-line function definitions with static on separate line > > > $ rg '^static \w+[^\(\{;]*$' | wc -l > > > 12 > > > > > > So I don't mind changing, but which one is canonical way of formatting? > > > > Not really, just my preference :) > > I'll stick to majority :) I feel like it's also a preferred style in > libbpf, so I'd rather converge to that. Majority is often wrong or at least lazy. But yeah, this is a waste of time. Do whatever. You also use inline keyword in C files in your libbpf patches.. I think kernel style rules should apply. > > In my experience having the return type on a separate line if its > > longer than a few chars is the simplest rule for consistent and good > > looking code. > > > > > > > + 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 (id = 1; id <= cnt; id++) { > > > > > + err = btf_dump__dump_type(d, id); > > > > > + if (err) > > > > > + goto done; > > > > > + } > > > > > + } > > > > > + > > > > > +done: > > > > > + btf_dump__free(d); > > > > > + return err; > > > > > > > > What do we do for JSON output? > > > > > > Still dump C syntax. What do you propose? Error out if json enabled? > > > > I wonder. Letting it just print C is going to confuse anything that > > just feeds the output into a JSON parser. I'd err on the side of > > returning an error, we can always relax that later if we find a use > > case of returning C syntax via JSON. > > Ok, I'll emit error (seems like pr_err automatically handles JSON > output, which is very nice). Thanks > > > > > if (!btf) { > > > > > err = btf__get_from_id(btf_id, &btf); > > > > > if (err) { > > > > > @@ -444,7 +498,10 @@ static int do_dump(int argc, char **argv) > > > > > } > > > > > } > > > > > > > > > > - dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > > > + if (dump_c) > > > > > + dump_btf_c(btf, root_type_ids, root_type_cnt); > > > > > + else > > > > > + dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > > > > > > > > done: > > > > > close(fd); > > > > > @@ -460,7 +517,7 @@ static int do_help(int argc, char **argv) > > > > > } > > > > > > > > > > fprintf(stderr, > > > > > - "Usage: %s btf dump BTF_SRC\n" > > > > > + "Usage: %s btf dump BTF_SRC [c]\n" > > > > > > > > bpftool generally uses <key value> formats. So perhaps we could do > > > > something like "[format raw|c]" here for consistency, defaulting to raw? > > > > > > That's not true for options, though. I see that at cgroup, prog, and > > > some map subcommands (haven't checked all other) just accept a list of > > > options without extra identifying key. > > > > Yeah, we weren't 100% enforcing this rule and it's a bit messy now :/ > > Unless you feel very strongly about this, it seems ok to me to allow > "boolean options" (similarly to boolean --flag args) as a stand-alone > set of tags. bpftool invocations are already very verbose, no need to > add to that. Plus it also makes bash-completion simpler, it's always > good not to complicate bash script unnecessarily :) It's more of a question if we're going to have more formats. If not then c as keyword is probably fine (although its worryingly short). If we start adding more then a key value would be better. Let's take a gamble and if we add 2 more output types I'll say "I told you so"? :)
On Thu, May 23, 2019 at 9:27 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 22 May 2019 21:43:43 -0700, Andrii Nakryiko wrote: > > On Wed, May 22, 2019 at 6:23 PM Jakub Kicinski wrote: > > > On Wed, 22 May 2019 17:58:23 -0700, Andrii Nakryiko wrote: > > > > On Wed, May 22, 2019 at 5:25 PM Jakub Kicinski wrote: > > > > > On Wed, 22 May 2019 12:50:51 -0700, Andrii Nakryiko wrote: > > > > > > + * Copyright (C) 2019 Facebook > > > > > > + */ > > > > > > > > > > > > #include <errno.h> > > > > > > #include <fcntl.h> > > > > > > @@ -340,11 +347,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) > > > > > > > > > > Please break the line after static int. > > > > > > > > I don't mind, but it seems that prevalent formatting for such cases > > > > (at least in bpftool code base) is aligning arguments and not break > > > > static <return type> into separate line: > > > > > > > > // multi-line function definitions with static on the same line > > > > $ rg '^static \w+.*\([^\)]*$' | wc -l > > > > 45 > > > > // multi-line function definitions with static on separate line > > > > $ rg '^static \w+[^\(\{;]*$' | wc -l > > > > 12 > > > > > > > > So I don't mind changing, but which one is canonical way of formatting? > > > > > > Not really, just my preference :) > > > > I'll stick to majority :) I feel like it's also a preferred style in > > libbpf, so I'd rather converge to that. > > Majority is often wrong or at least lazy. But yeah, this is a waste of > time. Do whatever. You also use inline keyword in C files in your > libbpf patches.. I think kernel style rules should apply. I'll remove inlines. > > > > In my experience having the return type on a separate line if its > > > longer than a few chars is the simplest rule for consistent and good > > > looking code. > > > > > > > > > + 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 (id = 1; id <= cnt; id++) { > > > > > > + err = btf_dump__dump_type(d, id); > > > > > > + if (err) > > > > > > + goto done; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > +done: > > > > > > + btf_dump__free(d); > > > > > > + return err; > > > > > > > > > > What do we do for JSON output? > > > > > > > > Still dump C syntax. What do you propose? Error out if json enabled? > > > > > > I wonder. Letting it just print C is going to confuse anything that > > > just feeds the output into a JSON parser. I'd err on the side of > > > returning an error, we can always relax that later if we find a use > > > case of returning C syntax via JSON. > > > > Ok, I'll emit error (seems like pr_err automatically handles JSON > > output, which is very nice). > > Thanks > > > > > > > if (!btf) { > > > > > > err = btf__get_from_id(btf_id, &btf); > > > > > > if (err) { > > > > > > @@ -444,7 +498,10 @@ static int do_dump(int argc, char **argv) > > > > > > } > > > > > > } > > > > > > > > > > > > - dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > > > > + if (dump_c) > > > > > > + dump_btf_c(btf, root_type_ids, root_type_cnt); > > > > > > + else > > > > > > + dump_btf_raw(btf, root_type_ids, root_type_cnt); > > > > > > > > > > > > done: > > > > > > close(fd); > > > > > > @@ -460,7 +517,7 @@ static int do_help(int argc, char **argv) > > > > > > } > > > > > > > > > > > > fprintf(stderr, > > > > > > - "Usage: %s btf dump BTF_SRC\n" > > > > > > + "Usage: %s btf dump BTF_SRC [c]\n" > > > > > > > > > > bpftool generally uses <key value> formats. So perhaps we could do > > > > > something like "[format raw|c]" here for consistency, defaulting to raw? > > > > > > > > That's not true for options, though. I see that at cgroup, prog, and > > > > some map subcommands (haven't checked all other) just accept a list of > > > > options without extra identifying key. > > > > > > Yeah, we weren't 100% enforcing this rule and it's a bit messy now :/ > > > > Unless you feel very strongly about this, it seems ok to me to allow > > "boolean options" (similarly to boolean --flag args) as a stand-alone > > set of tags. bpftool invocations are already very verbose, no need to > > add to that. Plus it also makes bash-completion simpler, it's always > > good not to complicate bash script unnecessarily :) > > It's more of a question if we're going to have more formats. If not > then c as keyword is probably fine (although its worryingly short). > If we start adding more then a key value would be better. Let's take a > gamble and if we add 2 more output types I'll say "I told you so"? :) Ok, that's fair :) There were talks about emitting Go, so it's not unimaginable that we'll have another format. I'll switch that to `format [c|raw]`.
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c index a22ef6587ebe..ed3d3221cc78 100644 --- a/tools/bpf/bpftool/btf.c +++ b/tools/bpf/bpftool/btf.c @@ -1,5 +1,12 @@ // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) -/* Copyright (C) 2019 Facebook */ + +/* + * BTF dumping command. + * Load BTF from multiple possible sources and outptu entirety or subset of + * types in either raw format or C-syntax format. + * + * Copyright (C) 2019 Facebook + */ #include <errno.h> #include <fcntl.h> @@ -340,11 +347,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, id; + + 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 (id = 1; id <= cnt; id++) { + err = btf_dump__dump_type(d, id); + 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 +475,16 @@ static int do_dump(int argc, char **argv) goto done; } + while (argc) { + if (strcmp(*argv, "c") == 0) { + dump_c = true; + 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 +498,10 @@ static int do_dump(int argc, char **argv) } } - dump_btf_raw(btf, root_type_ids, root_type_cnt); + if (dump_c) + dump_btf_c(btf, root_type_ids, root_type_cnt); + else + dump_btf_raw(btf, root_type_ids, root_type_cnt); done: close(fd); @@ -460,7 +517,7 @@ static int do_help(int argc, char **argv) } fprintf(stderr, - "Usage: %s btf dump BTF_SRC\n" + "Usage: %s btf dump BTF_SRC [c]\n" " %s btf help\n" "\n" " BTF_SRC := { id BTF_ID | prog PROG | map MAP [{key | value | kv | all}] | file FILE }\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 | 63 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-)