On Mon, May 23, 2022 at 11:05:08AM -0700, Andrii Nakryiko wrote:
> 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.
Okay, let's do that then.
Thanks,
Daniel
