On Mon, May 23, 2022 at 4:48 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> 2022-05-19 21:29 UTC+0000 ~ Daniel Müller <deso@xxxxxxxxxx>
> > 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.
> > We decided to fix up this deficiency with this change, meaning that
> > bpftool uses the same textual representations as libbpf. Supporting
> > test, completion scripts, and man pages have been adjusted accordingly.
> > However, we did add support for accepting (the now undocumented)
> > original attach type names when they are provided by users.
> >
> > For the test (test_bpftool_synctypes.py), I have removed the enum
> > representation checks, because we no longer mirror the various enum
> > variant names in bpftool source code. For the man page, help text, and
> > completion script checks we are now using enum definitions from
> > uapi/linux/bpf.h as the source of truth directly.
> >
> > Signed-off-by: Daniel Müller <deso@xxxxxxxxxx>
> > ---
> > .../bpftool/Documentation/bpftool-cgroup.rst | 16 +-
> > .../bpftool/Documentation/bpftool-prog.rst | 5 +-
> > tools/bpf/bpftool/bash-completion/bpftool | 18 +-
> > tools/bpf/bpftool/cgroup.c | 49 ++++--
> > tools/bpf/bpftool/common.c | 82 ++++-----
> > tools/bpf/bpftool/link.c | 15 +-
> > tools/bpf/bpftool/main.h | 17 ++
> > tools/bpf/bpftool/prog.c | 26 ++-
> > .../selftests/bpf/test_bpftool_synctypes.py | 163 ++++++++----------
> > 9 files changed, 213 insertions(+), 178 deletions(-)
[...]
> > 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 = libbpf_bpf_attach_type_str(type);
> > + if (!attach_type_str)
> > + break;
> > + if (is_prefix(str, attach_type_str))
>
> With so many shared prefixes here, I'm wondering if it would make more
> sense to compare the whole string instead? Otherwise it's hard to guess
> which type “bpftool c a <cgroup> cgroup_ <prog>” will use. At the same
> time we allow prefixing arguments everywhere else, so maybe not worth
> changing it here. Or we could maybe error out if the string length is <=
> strlen("cgroup_")? Let's see for a follow-up maybe.
>
Let's make string match exact for new strings and keep is_prefix()
logic for legacy values? It's better to split this loop into two then,
one over new-style strings and then over legacy strings.
> > + return type;
> > +
> > + /* Also check traditionally used attach type strings. */
> > + attach_type_str = bpf_attach_type_input_str(type);
> > + if (!attach_type_str)
> > + continue;
> > + if (is_prefix(str, attach_type_str))
> > return type;
> > }
> >
>
[...]
