2022-05-17 18:54 UTC+0000 ~ Daniel Müller <deso@xxxxxxxxxx> > On Tue, May 17, 2022 at 03:18:41PM +0100, Quentin Monnet wrote: >> 2022-05-16 16:41 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> >>> On Mon, May 16, 2022 at 10:36 AM Daniel Müller <deso@xxxxxxxxxx> 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@xxxxxxxxxx> >>>> --- >>> >>> 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
