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.
> 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
