Re: [PATCH bpf-next 09/12] bpftool: Use libbpf_bpf_attach_type_str

[Quentin Monnet <quentin@isovalent.com>]

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