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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, May 23, 2022 at 4:48 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2022-05-19 21:29 UTC+0000 ~ Daniel Müller <deso@posteo.net>
> > 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@posteo.net>
> > ---
> >  .../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;
> >       }
> >
>

[...]