| Message ID | 20220516173540.3520665-10-deso@posteo.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Textual representation of enums | expand |
On Mon, May 16, 2022 at 10:36 AM Daniel Müller <deso@posteo.net> wrote: > > This change switches bpftool over to using the recently introduced > libbpf_bpf_attach_type_str function instead of maintaining its own > string representation for the bpf_attach_type enum. > > Note that contrary to other enum types, the variant names that bpftool > maps bpf_attach_type to do not follow a simple to follow rule. With > bpf_prog_type, for example, the textual representation can easily be > inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the > remaining string. bpf_attach_type violates this rule for various > variants. In order to not brake compatibility (these textual > representations appear in JSON and are used to parse user input), we > introduce a program local bpf_attach_type_str that overrides the > variants in question. > We should consider removing this function and expect the libbpf string > representation with the next backwards compatibility breaking release, > if possible. > > Signed-off-by: Daniel Müller <deso@posteo.net> > --- Quentin, any opinion on this approach? Should we fallback to libbpf's API for all the future cases or it's better to keep bpftool's own attach_type mapping? > tools/bpf/bpftool/cgroup.c | 20 ++++++---- > tools/bpf/bpftool/common.c | 82 +++++++++++++++++--------------------- > tools/bpf/bpftool/link.c | 15 ++++--- > tools/bpf/bpftool/main.h | 15 +++++++ > 4 files changed, 73 insertions(+), 59 deletions(-) > [...]
2022-05-16 16:41 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Mon, May 16, 2022 at 10:36 AM Daniel Müller <deso@posteo.net> wrote: >> >> This change switches bpftool over to using the recently introduced >> libbpf_bpf_attach_type_str function instead of maintaining its own >> string representation for the bpf_attach_type enum. >> >> Note that contrary to other enum types, the variant names that bpftool >> maps bpf_attach_type to do not follow a simple to follow rule. With >> bpf_prog_type, for example, the textual representation can easily be >> inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the >> remaining string. bpf_attach_type violates this rule for various >> variants. In order to not brake compatibility (these textual >> representations appear in JSON and are used to parse user input), we >> introduce a program local bpf_attach_type_str that overrides the >> variants in question. >> We should consider removing this function and expect the libbpf string >> representation with the next backwards compatibility breaking release, >> if possible. >> >> Signed-off-by: Daniel Müller <deso@posteo.net> >> --- > > Quentin, any opinion on this approach? Should we fallback to libbpf's > API for all the future cases or it's better to keep bpftool's own > attach_type mapping? Hi Andrii, Daniel, Thanks for the ping! I'm unsure what's best, to be honest. Maybe we should look at bpftool's inputs and outputs separately. For attach types printed as part of the output: Thinking about it, I'd say go for the libbpf API, and make the change now. As much as we all dislike breaking things for user space, I believe that on the long term, we would benefit from having a more consistent naming scheme for those strings (prefix + lowercase attach type); and more importantly, if querying the string from libbpf spreads to other tools, these will be the reference strings for the attach types and it will be a pain to convert bpftool's specific exceptions to "regular" textual representations to interface with other tools. And if we must break things, I'd as well have it synchronised with the release of libbpf 1.0, so I'd say let's just change it now? Note that we're now tagging bpftool releases on the GitHub mirror (I did 6.8.0 earlier today), so at least that's one place where we can have a changelog and mention breaking changes. Now for the attach types parsed as input parameters: I wonder if it would be worth supporting the two values for attach types where they differ, so that we would avoid breaking bpftool commands themselves? It's a bit more code, but then the list would be relatively short, and not expected to grow. We can update the documentation to mention only the new names, and just keep the short compat list hidden. Some additional notes on the patch: There is also attach_type_strings[] in prog.c where strings for attach types are re-defined, this time for when non cgroup-related programs are attached (through "bpftool prog attach"). It's used for parsing the input, so should be treated the same as the attach list in commons.c. If changing the attach type names, we should also update the following: - man pages: tools/bpf/bpftool/Documentation/bpftool-{cgroup,prog}.rst - interactive help (cgroup.c:HELP_SPEC_ATTACH_TYPES + prog.c:do_help()) - bash completion: tools/bpf/bpftool/bash-completion/bpftool Some of the tests in tools/testing/selftests/bpf/test_bpftool_synctypes.py, related to keeping those lists up-to-date, will also need to be modified to parse the names from libbpf instead of bpftool sources. I can help with that if necessary. Quentin
On Tue, May 17, 2022 at 03:18:41PM +0100, Quentin Monnet wrote: > 2022-05-16 16:41 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > On Mon, May 16, 2022 at 10:36 AM Daniel Müller <deso@posteo.net> wrote: > >> > >> This change switches bpftool over to using the recently introduced > >> libbpf_bpf_attach_type_str function instead of maintaining its own > >> string representation for the bpf_attach_type enum. > >> > >> Note that contrary to other enum types, the variant names that bpftool > >> maps bpf_attach_type to do not follow a simple to follow rule. With > >> bpf_prog_type, for example, the textual representation can easily be > >> inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the > >> remaining string. bpf_attach_type violates this rule for various > >> variants. In order to not brake compatibility (these textual > >> representations appear in JSON and are used to parse user input), we > >> introduce a program local bpf_attach_type_str that overrides the > >> variants in question. > >> We should consider removing this function and expect the libbpf string > >> representation with the next backwards compatibility breaking release, > >> if possible. > >> > >> Signed-off-by: Daniel Müller <deso@posteo.net> > >> --- > > > > Quentin, any opinion on this approach? Should we fallback to libbpf's > > API for all the future cases or it's better to keep bpftool's own > > attach_type mapping? > Hi Andrii, Daniel, Hi Quentin, > Thanks for the ping! I'm unsure what's best, to be honest. Maybe we > should look at bpftool's inputs and outputs separately. > > For attach types printed as part of the output: > > Thinking about it, I'd say go for the libbpf API, and make the change > now. As much as we all dislike breaking things for user space, I believe > that on the long term, we would benefit from having a more consistent > naming scheme for those strings (prefix + lowercase attach type); and > more importantly, if querying the string from libbpf spreads to other > tools, these will be the reference strings for the attach types and it > will be a pain to convert bpftool's specific exceptions to "regular" > textual representations to interface with other tools. > > And if we must break things, I'd as well have it synchronised with the > release of libbpf 1.0, so I'd say let's just change it now? Note that > we're now tagging bpftool releases on the GitHub mirror (I did 6.8.0 > earlier today), so at least that's one place where we can have a > changelog and mention breaking changes. Thanks for the feedback. This sounds good to me. I can make the change. But do you think we should do it as part of this stack? We could make this very stack a behavior preserving step (that can reasonably stand on its own). Given the additional changes to tests & documentation that you mention below, it would make sense to me to keep individual patches in this series similar in nature. I'd be happy to author a follow-on, but can also amend this series if that's preferred. Please let me know your preference. > Now for the attach types parsed as input parameters: > > I wonder if it would be worth supporting the two values for attach types > where they differ, so that we would avoid breaking bpftool commands > themselves? It's a bit more code, but then the list would be relatively > short, and not expected to grow. We can update the documentation to > mention only the new names, and just keep the short compat list hidden. Yes, that should be easily possible. I do have a similar question to above, though (as it involves updating documentation for new preference): can/should that be a separate patch? > Some additional notes on the patch: > > There is also attach_type_strings[] in prog.c where strings for attach > types are re-defined, this time for when non cgroup-related programs are > attached (through "bpftool prog attach"). It's used for parsing the > input, so should be treated the same as the attach list in commons.c. Good point. If we want to use libbpf text representations moving forward then I can adjust this array as well. Do I assume correctly that we would want to keep the existing variants as hidden fallbacks here too, as you mentioned above? > If changing the attach type names, we should also update the following: > - man pages: tools/bpf/bpftool/Documentation/bpftool-{cgroup,prog}.rst > - interactive help (cgroup.c:HELP_SPEC_ATTACH_TYPES + prog.c:do_help()) > - bash completion: tools/bpf/bpftool/bash-completion/bpftool > > Some of the tests in > tools/testing/selftests/bpf/test_bpftool_synctypes.py, related to > keeping those lists up-to-date, will also need to be modified to parse > the names from libbpf instead of bpftool sources. I can help with that > if necessary. Sounds good; will do that here or as a follow-on (and reach out to you if I need assistance), depending on your preference as inquired above. Thanks, Daniel
2022-05-17 18:54 UTC+0000 ~ Daniel Müller <deso@posteo.net> > On Tue, May 17, 2022 at 03:18:41PM +0100, Quentin Monnet wrote: >> 2022-05-16 16:41 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> >>> On Mon, May 16, 2022 at 10:36 AM Daniel Müller <deso@posteo.net> wrote: >>>> >>>> This change switches bpftool over to using the recently introduced >>>> libbpf_bpf_attach_type_str function instead of maintaining its own >>>> string representation for the bpf_attach_type enum. >>>> >>>> Note that contrary to other enum types, the variant names that bpftool >>>> maps bpf_attach_type to do not follow a simple to follow rule. With >>>> bpf_prog_type, for example, the textual representation can easily be >>>> inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the >>>> remaining string. bpf_attach_type violates this rule for various >>>> variants. In order to not brake compatibility (these textual >>>> representations appear in JSON and are used to parse user input), we >>>> introduce a program local bpf_attach_type_str that overrides the >>>> variants in question. >>>> We should consider removing this function and expect the libbpf string >>>> representation with the next backwards compatibility breaking release, >>>> if possible. >>>> >>>> Signed-off-by: Daniel Müller <deso@posteo.net> >>>> --- >>> >>> Quentin, any opinion on this approach? Should we fallback to libbpf's >>> API for all the future cases or it's better to keep bpftool's own >>> attach_type mapping? >> Hi Andrii, Daniel, > > Hi Quentin, > >> Thanks for the ping! I'm unsure what's best, to be honest. Maybe we >> should look at bpftool's inputs and outputs separately. >> >> For attach types printed as part of the output: >> >> Thinking about it, I'd say go for the libbpf API, and make the change >> now. As much as we all dislike breaking things for user space, I believe >> that on the long term, we would benefit from having a more consistent >> naming scheme for those strings (prefix + lowercase attach type); and >> more importantly, if querying the string from libbpf spreads to other >> tools, these will be the reference strings for the attach types and it >> will be a pain to convert bpftool's specific exceptions to "regular" >> textual representations to interface with other tools. >> >> And if we must break things, I'd as well have it synchronised with the >> release of libbpf 1.0, so I'd say let's just change it now? Note that >> we're now tagging bpftool releases on the GitHub mirror (I did 6.8.0 >> earlier today), so at least that's one place where we can have a >> changelog and mention breaking changes. > > Thanks for the feedback. This sounds good to me. I can make the change. But do > you think we should do it as part of this stack? We could make this very stack a > behavior preserving step (that can reasonably stand on its own). Given the > additional changes to tests & documentation that you mention below, it would > make sense to me to keep individual patches in this series similar in nature. > I'd be happy to author a follow-on, but can also amend this series if that's > preferred. Please let me know your preference. So I would have a slight preference for having everything together, but at the same time I understand that the current patchset is already in a good state and that you don't want to "overgrow" it too much. So follow-up is fine by me, mostly (see next paragraph), if it lands before libbpf v1.0 is released. If I understand correctly, you would have the addition of the new names as an alternative for input parameters in this set, and follow-up with the breaking changes (using the new names for output, and switching to new names for input in docs) as a follow-up, is this correct? Still, the tests in test_bpftool_synctypes.py should be updated in this PR, because the bpftool tests in the CI break otherwise [0]. This is due to the removal of the lists of names in bpftool: the test parses them to compare the lists with what's present in UAPI bpf.h, bpftool man pages, etc. I believe it would be best to keep this in a running state. [0] https://github.com/kernel-patches/bpf/runs/6459959177?check_suite_focus=true#step:6:678 > >> Now for the attach types parsed as input parameters: >> >> I wonder if it would be worth supporting the two values for attach types >> where they differ, so that we would avoid breaking bpftool commands >> themselves? It's a bit more code, but then the list would be relatively >> short, and not expected to grow. We can update the documentation to >> mention only the new names, and just keep the short compat list hidden. > > Yes, that should be easily possible. I do have a similar question to above, > though (as it involves updating documentation for new preference): can/should > that be a separate patch? > >> Some additional notes on the patch: >> >> There is also attach_type_strings[] in prog.c where strings for attach >> types are re-defined, this time for when non cgroup-related programs are >> attached (through "bpftool prog attach"). It's used for parsing the >> input, so should be treated the same as the attach list in commons.c. > > Good point. If we want to use libbpf text representations moving forward then I > can adjust this array as well. Do I assume correctly that we would want to keep > the existing variants as hidden fallbacks here too, as you mentioned above? Yes, exactly. >> If changing the attach type names, we should also update the following: >> - man pages: tools/bpf/bpftool/Documentation/bpftool-{cgroup,prog}.rst >> - interactive help (cgroup.c:HELP_SPEC_ATTACH_TYPES + prog.c:do_help()) >> - bash completion: tools/bpf/bpftool/bash-completion/bpftool >> >> Some of the tests in >> tools/testing/selftests/bpf/test_bpftool_synctypes.py, related to >> keeping those lists up-to-date, will also need to be modified to parse >> the names from libbpf instead of bpftool sources. I can help with that >> if necessary. > > Sounds good; will do that here or as a follow-on (and reach out to you if I need > assistance), depending on your preference as inquired above. Sounds like a plan :) Thanks, Quentin
On Wed, May 18, 2022 at 02:31:40PM +0100, Quentin Monnet wrote: > 2022-05-17 18:54 UTC+0000 ~ Daniel Müller <deso@posteo.net> > > On Tue, May 17, 2022 at 03:18:41PM +0100, Quentin Monnet wrote: > >> 2022-05-16 16:41 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > >>> On Mon, May 16, 2022 at 10:36 AM Daniel Müller <deso@posteo.net> wrote: > >>>> > >>>> This change switches bpftool over to using the recently introduced > >>>> libbpf_bpf_attach_type_str function instead of maintaining its own > >>>> string representation for the bpf_attach_type enum. > >>>> > >>>> Note that contrary to other enum types, the variant names that bpftool > >>>> maps bpf_attach_type to do not follow a simple to follow rule. With > >>>> bpf_prog_type, for example, the textual representation can easily be > >>>> inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the > >>>> remaining string. bpf_attach_type violates this rule for various > >>>> variants. In order to not brake compatibility (these textual > >>>> representations appear in JSON and are used to parse user input), we > >>>> introduce a program local bpf_attach_type_str that overrides the > >>>> variants in question. > >>>> We should consider removing this function and expect the libbpf string > >>>> representation with the next backwards compatibility breaking release, > >>>> if possible. > >>>> > >>>> Signed-off-by: Daniel Müller <deso@posteo.net> > >>>> --- > >>> > >>> Quentin, any opinion on this approach? Should we fallback to libbpf's > >>> API for all the future cases or it's better to keep bpftool's own > >>> attach_type mapping? > >> Hi Andrii, Daniel, > > > > Hi Quentin, > > > >> Thanks for the ping! I'm unsure what's best, to be honest. Maybe we > >> should look at bpftool's inputs and outputs separately. > >> > >> For attach types printed as part of the output: > >> > >> Thinking about it, I'd say go for the libbpf API, and make the change > >> now. As much as we all dislike breaking things for user space, I believe > >> that on the long term, we would benefit from having a more consistent > >> naming scheme for those strings (prefix + lowercase attach type); and > >> more importantly, if querying the string from libbpf spreads to other > >> tools, these will be the reference strings for the attach types and it > >> will be a pain to convert bpftool's specific exceptions to "regular" > >> textual representations to interface with other tools. > >> > >> And if we must break things, I'd as well have it synchronised with the > >> release of libbpf 1.0, so I'd say let's just change it now? Note that > >> we're now tagging bpftool releases on the GitHub mirror (I did 6.8.0 > >> earlier today), so at least that's one place where we can have a > >> changelog and mention breaking changes. > > > > Thanks for the feedback. This sounds good to me. I can make the change. But do > > you think we should do it as part of this stack? We could make this very stack a > > behavior preserving step (that can reasonably stand on its own). Given the > > additional changes to tests & documentation that you mention below, it would > > make sense to me to keep individual patches in this series similar in nature. > > I'd be happy to author a follow-on, but can also amend this series if that's > > preferred. Please let me know your preference. > > So I would have a slight preference for having everything together, but > at the same time I understand that the current patchset is already in > a good state and that you don't want to "overgrow" it too much. So > follow-up is fine by me, mostly (see next paragraph), if it lands before > libbpf v1.0 is released. If I understand correctly, you would have the > addition of the new names as an alternative for input parameters in this > set, and follow-up with the breaking changes (using the new names for > output, and switching to new names for input in docs) as a follow-up, is > this correct? Correct, that was my initial plan. But I had a look at the test code and given how much it relies on the current source code (without this patch), my suggested approach is actually no longer tenable (or at least would require significantly more work, I'd say). I am now adjusting everything in this patch and also added the fallbacks as you suggested. > Still, the tests in test_bpftool_synctypes.py should be updated in this > PR, because the bpftool tests in the CI break otherwise [0]. This is due > to the removal of the lists of names in bpftool: the test parses them to > compare the lists with what's present in UAPI bpf.h, bpftool man pages, > etc. I believe it would be best to keep this in a running state. > > [0] > https://github.com/kernel-patches/bpf/runs/6459959177?check_suite_focus=true#step:6:678 Thanks for the pointer. I've updated the test. It has become a rather large patch now, though. What I ended up doing is to mostly rely on uapi/linux/bpf.h enum definitions as the source of truth. Let me know if you want me to poke into libbpf instead. I'd honestly rather avoid doing that. Are there additional tests that I should run to give me some more confidence in my changes? I tried running test_bpftool.sh, but it fails for a reason not apparent to me. I did not investigate, because it also fails on master and one of the commands that failed, bpftool feature probe macros, when run directly, succeeds both with and without my patch set (which strongly suggests to me that there is something else amiss here). > >> Now for the attach types parsed as input parameters: > >> > >> I wonder if it would be worth supporting the two values for attach types > >> where they differ, so that we would avoid breaking bpftool commands > >> themselves? It's a bit more code, but then the list would be relatively > >> short, and not expected to grow. We can update the documentation to > >> mention only the new names, and just keep the short compat list hidden. > > > > Yes, that should be easily possible. I do have a similar question to above, > > though (as it involves updating documentation for new preference): can/should > > that be a separate patch? > > > >> Some additional notes on the patch: > >> > >> There is also attach_type_strings[] in prog.c where strings for attach > >> types are re-defined, this time for when non cgroup-related programs are > >> attached (through "bpftool prog attach"). It's used for parsing the > >> input, so should be treated the same as the attach list in commons.c. > > > > Good point. If we want to use libbpf text representations moving forward then I > > can adjust this array as well. Do I assume correctly that we would want to keep > > the existing variants as hidden fallbacks here too, as you mentioned above? > > Yes, exactly. I've adjusted this array now as well. > >> If changing the attach type names, we should also update the following: > >> - man pages: tools/bpf/bpftool/Documentation/bpftool-{cgroup,prog}.rst > >> - interactive help (cgroup.c:HELP_SPEC_ATTACH_TYPES + prog.c:do_help()) > >> - bash completion: tools/bpf/bpftool/bash-completion/bpftool > >> > >> Some of the tests in > >> tools/testing/selftests/bpf/test_bpftool_synctypes.py, related to > >> keeping those lists up-to-date, will also need to be modified to parse > >> the names from libbpf instead of bpftool sources. I can help with that > >> if necessary. > > > > Sounds good; will do that here or as a follow-on (and reach out to you if I need > > assistance), depending on your preference as inquired above. > > Sounds like a plan :) Done. Though I did go with bpf.h as the source of truth. Please let me know if you want me to use libbpf instead. Will update to v2 soon. Thanks, Daniel
> -----Original Message----- > From: Quentin Monnet <quentin@isovalent.com> > Sent: Wednesday, May 18, 2022 6:32 AM > To: Daniel Müller <deso@posteo.net> > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>; bpf <bpf@vger.kernel.org>; > Alexei Starovoitov <ast@kernel.org>; Andrii Nakryiko <andrii@kernel.org>; > Daniel Borkmann <daniel@iogearbox.net>; Kernel Team <kernel- > team@fb.com> > Subject: Re: [PATCH bpf-next 09/12] bpftool: Use libbpf_bpf_attach_type_str > > 2022-05-17 18:54 UTC+0000 ~ Daniel Müller <deso@posteo.net> > > On Tue, May 17, 2022 at 03:18:41PM +0100, Quentin Monnet wrote: > >> 2022-05-16 16:41 UTC-0700 ~ Andrii Nakryiko > >> <andrii.nakryiko@gmail.com> > >>> On Mon, May 16, 2022 at 10:36 AM Daniel Müller <deso@posteo.net> > wrote: > >>>> > >>>> This change switches bpftool over to using the recently introduced > >>>> libbpf_bpf_attach_type_str function instead of maintaining its own > >>>> string representation for the bpf_attach_type enum. > >>>> > >>>> Note that contrary to other enum types, the variant names that > >>>> bpftool maps bpf_attach_type to do not follow a simple to follow > >>>> rule. With bpf_prog_type, for example, the textual representation > >>>> can easily be inferred by stripping the BPF_PROG_TYPE_ prefix and > >>>> lowercasing the remaining string. bpf_attach_type violates this > >>>> rule for various variants. In order to not brake compatibility > >>>> (these textual representations appear in JSON and are used to parse > >>>> user input), we introduce a program local bpf_attach_type_str that > >>>> overrides the variants in question. > >>>> We should consider removing this function and expect the libbpf > >>>> string representation with the next backwards compatibility > >>>> breaking release, if possible. > >>>> > >>>> Signed-off-by: Daniel Müller <deso@posteo.net> > >>>> --- > >>> > >>> Quentin, any opinion on this approach? Should we fallback to > >>> libbpf's API for all the future cases or it's better to keep > >>> bpftool's own attach_type mapping? > >> Hi Andrii, Daniel, > > > > Hi Quentin, > > > >> Thanks for the ping! I'm unsure what's best, to be honest. Maybe we > >> should look at bpftool's inputs and outputs separately. > >> > >> For attach types printed as part of the output: > >> > >> Thinking about it, I'd say go for the libbpf API, and make the change > >> now. [...] I mentioned this at LSF/MM/BPF in the discussion of making bpftool & libbpf cross-platform, but yes it is a good direction (in terms of supporting more runtimes) to avoid as much as possible having per-prog/attach type info in bpftool itself. So yes depending on libbpf apis instead of hard coding in bpftool is good. Dave
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c index effe136..fdaef2 100644 --- a/tools/bpf/bpftool/cgroup.c +++ b/tools/bpf/bpftool/cgroup.c @@ -35,11 +35,14 @@ static unsigned int query_flags; static enum bpf_attach_type parse_attach_type(const char *str) { + const char *attach_type_str; enum bpf_attach_type type; - for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) { - if (attach_type_name[type] && - is_prefix(str, attach_type_name[type])) + for (type = 0; ; type++) { + attach_type_str = bpf_attach_type_str(type); + if (!attach_type_str) + break; + if (is_prefix(str, attach_type_str)) return type; } @@ -52,6 +55,7 @@ static int show_bpf_prog(int id, enum bpf_attach_type attach_type, { char prog_name[MAX_PROG_FULL_NAME]; struct bpf_prog_info info = {}; + const char *attach_type_str; __u32 info_len = sizeof(info); int prog_fd; @@ -64,13 +68,13 @@ static int show_bpf_prog(int id, enum bpf_attach_type attach_type, return -1; } + attach_type_str = bpf_attach_type_str(attach_type); get_prog_full_name(&info, prog_fd, prog_name, sizeof(prog_name)); if (json_output) { jsonw_start_object(json_wtr); jsonw_uint_field(json_wtr, "id", info.id); - if (attach_type < ARRAY_SIZE(attach_type_name)) - jsonw_string_field(json_wtr, "attach_type", - attach_type_name[attach_type]); + if (attach_type_str) + jsonw_string_field(json_wtr, "attach_type", attach_type_str); else jsonw_uint_field(json_wtr, "attach_type", attach_type); jsonw_string_field(json_wtr, "attach_flags", @@ -79,8 +83,8 @@ static int show_bpf_prog(int id, enum bpf_attach_type attach_type, jsonw_end_object(json_wtr); } else { printf("%s%-8u ", level ? " " : "", info.id); - if (attach_type < ARRAY_SIZE(attach_type_name)) - printf("%-15s", attach_type_name[attach_type]); + if (attach_type_str) + printf("%-15s", attach_type_str); else printf("type %-10u", attach_type); printf(" %-15s %-15s\n", attach_flags_str, prog_name); diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index c74014..00ab85 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -31,52 +31,6 @@ #define BPF_FS_MAGIC 0xcafe4a11 #endif -const char * const attach_type_name[__MAX_BPF_ATTACH_TYPE] = { - [BPF_CGROUP_INET_INGRESS] = "ingress", - [BPF_CGROUP_INET_EGRESS] = "egress", - [BPF_CGROUP_INET_SOCK_CREATE] = "sock_create", - [BPF_CGROUP_INET_SOCK_RELEASE] = "sock_release", - [BPF_CGROUP_SOCK_OPS] = "sock_ops", - [BPF_CGROUP_DEVICE] = "device", - [BPF_CGROUP_INET4_BIND] = "bind4", - [BPF_CGROUP_INET6_BIND] = "bind6", - [BPF_CGROUP_INET4_CONNECT] = "connect4", - [BPF_CGROUP_INET6_CONNECT] = "connect6", - [BPF_CGROUP_INET4_POST_BIND] = "post_bind4", - [BPF_CGROUP_INET6_POST_BIND] = "post_bind6", - [BPF_CGROUP_INET4_GETPEERNAME] = "getpeername4", - [BPF_CGROUP_INET6_GETPEERNAME] = "getpeername6", - [BPF_CGROUP_INET4_GETSOCKNAME] = "getsockname4", - [BPF_CGROUP_INET6_GETSOCKNAME] = "getsockname6", - [BPF_CGROUP_UDP4_SENDMSG] = "sendmsg4", - [BPF_CGROUP_UDP6_SENDMSG] = "sendmsg6", - [BPF_CGROUP_SYSCTL] = "sysctl", - [BPF_CGROUP_UDP4_RECVMSG] = "recvmsg4", - [BPF_CGROUP_UDP6_RECVMSG] = "recvmsg6", - [BPF_CGROUP_GETSOCKOPT] = "getsockopt", - [BPF_CGROUP_SETSOCKOPT] = "setsockopt", - [BPF_SK_SKB_STREAM_PARSER] = "sk_skb_stream_parser", - [BPF_SK_SKB_STREAM_VERDICT] = "sk_skb_stream_verdict", - [BPF_SK_SKB_VERDICT] = "sk_skb_verdict", - [BPF_SK_MSG_VERDICT] = "sk_msg_verdict", - [BPF_LIRC_MODE2] = "lirc_mode2", - [BPF_FLOW_DISSECTOR] = "flow_dissector", - [BPF_TRACE_RAW_TP] = "raw_tp", - [BPF_TRACE_FENTRY] = "fentry", - [BPF_TRACE_FEXIT] = "fexit", - [BPF_MODIFY_RETURN] = "mod_ret", - [BPF_LSM_MAC] = "lsm_mac", - [BPF_SK_LOOKUP] = "sk_lookup", - [BPF_TRACE_ITER] = "trace_iter", - [BPF_XDP_DEVMAP] = "xdp_devmap", - [BPF_XDP_CPUMAP] = "xdp_cpumap", - [BPF_XDP] = "xdp", - [BPF_SK_REUSEPORT_SELECT] = "sk_skb_reuseport_select", - [BPF_SK_REUSEPORT_SELECT_OR_MIGRATE] = "sk_skb_reuseport_select_or_migrate", - [BPF_PERF_EVENT] = "perf_event", - [BPF_TRACE_KPROBE_MULTI] = "trace_kprobe_multi", -}; - void p_err(const char *fmt, ...) { va_list ap; @@ -1009,3 +963,39 @@ bool equal_fn_for_key_as_id(const void *k1, const void *k2, void *ctx) { return k1 == k2; } + +const char *bpf_attach_type_str(enum bpf_attach_type t) +{ + switch (t) { + case BPF_CGROUP_INET_INGRESS: return "ingress"; + case BPF_CGROUP_INET_EGRESS: return "egress"; + case BPF_CGROUP_INET_SOCK_CREATE: return "sock_create"; + case BPF_CGROUP_INET_SOCK_RELEASE: return "sock_release"; + case BPF_CGROUP_SOCK_OPS: return "sock_ops"; + case BPF_CGROUP_DEVICE: return "device"; + case BPF_CGROUP_INET4_BIND: return "bind4"; + case BPF_CGROUP_INET6_BIND: return "bind6"; + case BPF_CGROUP_INET4_CONNECT: return "connect4"; + case BPF_CGROUP_INET6_CONNECT: return "connect6"; + case BPF_CGROUP_INET4_POST_BIND: return "post_bind4"; + case BPF_CGROUP_INET6_POST_BIND: return "post_bind6"; + case BPF_CGROUP_INET4_GETPEERNAME: return "getpeername4"; + case BPF_CGROUP_INET6_GETPEERNAME: return "getpeername6"; + case BPF_CGROUP_INET4_GETSOCKNAME: return "getsockname4"; + case BPF_CGROUP_INET6_GETSOCKNAME: return "getsockname6"; + case BPF_CGROUP_UDP4_SENDMSG: return "sendmsg4"; + case BPF_CGROUP_UDP6_SENDMSG: return "sendmsg6"; + case BPF_CGROUP_SYSCTL: return "sysctl"; + case BPF_CGROUP_UDP4_RECVMSG: return "recvmsg4"; + case BPF_CGROUP_UDP6_RECVMSG: return "recvmsg6"; + case BPF_CGROUP_GETSOCKOPT: return "getsockopt"; + case BPF_CGROUP_SETSOCKOPT: return "setsockopt"; + case BPF_TRACE_RAW_TP: return "raw_tp"; + case BPF_TRACE_FENTRY: return "fentry"; + case BPF_TRACE_FEXIT: return "fexit"; + case BPF_MODIFY_RETURN: return "mod_ret"; + case BPF_SK_REUSEPORT_SELECT: return "sk_skb_reuseport_select"; + case BPF_SK_REUSEPORT_SELECT_OR_MIGRATE: return "sk_skb_reuseport_select_or_migrate"; + default: return libbpf_bpf_attach_type_str(t); + } +} diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c index e27108..2dd0d01 100644 --- a/tools/bpf/bpftool/link.c +++ b/tools/bpf/bpftool/link.c @@ -78,9 +78,11 @@ show_link_header_json(struct bpf_link_info *info, json_writer_t *wtr) static void show_link_attach_type_json(__u32 attach_type, json_writer_t *wtr) { - if (attach_type < ARRAY_SIZE(attach_type_name)) - jsonw_string_field(wtr, "attach_type", - attach_type_name[attach_type]); + const char *attach_type_str; + + attach_type_str = bpf_attach_type_str(attach_type); + if (attach_type_str) + jsonw_string_field(wtr, "attach_type", attach_type_str); else jsonw_uint_field(wtr, "attach_type", attach_type); } @@ -196,8 +198,11 @@ static void show_link_header_plain(struct bpf_link_info *info) static void show_link_attach_type_plain(__u32 attach_type) { - if (attach_type < ARRAY_SIZE(attach_type_name)) - printf("attach_type %s ", attach_type_name[attach_type]); + const char *attach_type_str; + + attach_type_str = bpf_attach_type_str(attach_type); + if (attach_type_str) + printf("attach_type %s ", attach_type_str); else printf("attach_type %u ", attach_type); } diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index e4fdaa0..e27237c 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -243,6 +243,21 @@ int print_all_levels(__maybe_unused enum libbpf_print_level level, size_t hash_fn_for_key_as_id(const void *key, void *ctx); bool equal_fn_for_key_as_id(const void *k1, const void *k2, void *ctx); +/** + * bpf_attach_type_str - convert the provided attach type value into a textual + * representation. + * + * This function acts as a wrapper around libbpf_bpf_attach_type_str which + * overrides some of the variant names with ones that have traditionally been + * used in the program (and that do not follow the string inference scheme that + * libbpf uses). + * + * @t: The attach type + * Returns a pointer to a static string identifying the attach type. NULL is + * returned for unknown bpf_attach_type values. + */ +const char *bpf_attach_type_str(enum bpf_attach_type t); + static inline void *u32_as_hash_field(__u32 x) { return (void *)(uintptr_t)x;
This change switches bpftool over to using the recently introduced libbpf_bpf_attach_type_str function instead of maintaining its own string representation for the bpf_attach_type enum. Note that contrary to other enum types, the variant names that bpftool maps bpf_attach_type to do not follow a simple to follow rule. With bpf_prog_type, for example, the textual representation can easily be inferred by stripping the BPF_PROG_TYPE_ prefix and lowercasing the remaining string. bpf_attach_type violates this rule for various variants. In order to not brake compatibility (these textual representations appear in JSON and are used to parse user input), we introduce a program local bpf_attach_type_str that overrides the variants in question. We should consider removing this function and expect the libbpf string representation with the next backwards compatibility breaking release, if possible. Signed-off-by: Daniel Müller <deso@posteo.net> --- tools/bpf/bpftool/cgroup.c | 20 ++++++---- tools/bpf/bpftool/common.c | 82 +++++++++++++++++--------------------- tools/bpf/bpftool/link.c | 15 ++++--- tools/bpf/bpftool/main.h | 15 +++++++ 4 files changed, 73 insertions(+), 59 deletions(-)