Re: [PATCH bpf-next 0/7] tools: bpftool: update, synchronise and

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

On Thu, Jul 29, 2021 at 9:29 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> To work with the different program types, map types, attach types etc.
> supported by eBPF, bpftool needs occasional updates to learn about the new
> features supported by the kernel. When such types translate into new
> keyword for the command line, updates are expected in several locations:
> typically, the help message displayed from bpftool itself, the manual page,
> and the bash completion file should be updated. The options used by the
> different commands for bpftool should also remain synchronised at those
> locations.
>
> Several omissions have occurred in the past, and a number of types are
> still missing today. This set is an attempt to improve the situation. It
> brings up-to-date the lists of types or options in bpftool, and also adds a
> Python script to the BPF selftests to automatically check that most of
> these lists remain synchronised.
>
> Quentin Monnet (7):
>   tools: bpftool: slightly ease bash completion updates
>   selftests/bpf: check consistency between bpftool source, doc,
>     completion
>   tools: bpftool: complete and synchronise attach or map types
>   tools: bpftool: update and synchronise option list in doc and help msg
>   selftests/bpf: update bpftool's consistency script for checking
>     options
>   tools: bpftool: document and add bash completion for -L, -B options
>   tools: bpftool: complete metrics list in "bpftool prog profile" doc
>
>  .../bpf/bpftool/Documentation/bpftool-btf.rst |  48 +-
>  .../bpftool/Documentation/bpftool-cgroup.rst  |   3 +-
>  .../bpftool/Documentation/bpftool-feature.rst |   2 +-
>  .../bpf/bpftool/Documentation/bpftool-gen.rst |   9 +-
>  .../bpftool/Documentation/bpftool-iter.rst    |   2 +
>  .../bpftool/Documentation/bpftool-link.rst    |   3 +-
>  .../bpf/bpftool/Documentation/bpftool-map.rst |   3 +-
>  .../bpf/bpftool/Documentation/bpftool-net.rst |   2 +-
>  .../bpftool/Documentation/bpftool-perf.rst    |   2 +-
>  .../bpftool/Documentation/bpftool-prog.rst    |  36 +-
>  .../Documentation/bpftool-struct_ops.rst      |   2 +-
>  tools/bpf/bpftool/Documentation/bpftool.rst   |  12 +-
>  tools/bpf/bpftool/bash-completion/bpftool     |  69 ++-
>  tools/bpf/bpftool/btf.c                       |   3 +-
>  tools/bpf/bpftool/cgroup.c                    |   3 +-
>  tools/bpf/bpftool/common.c                    |  76 +--
>  tools/bpf/bpftool/feature.c                   |   1 +
>  tools/bpf/bpftool/gen.c                       |   3 +-
>  tools/bpf/bpftool/iter.c                      |   2 +
>  tools/bpf/bpftool/link.c                      |   3 +-
>  tools/bpf/bpftool/main.c                      |   3 +-
>  tools/bpf/bpftool/main.h                      |   3 +-
>  tools/bpf/bpftool/map.c                       |   5 +-
>  tools/bpf/bpftool/net.c                       |   1 +
>  tools/bpf/bpftool/perf.c                      |   5 +-
>  tools/bpf/bpftool/prog.c                      |   8 +-
>  tools/bpf/bpftool/struct_ops.c                |   2 +-
>  tools/testing/selftests/bpf/Makefile          |   1 +
>  .../selftests/bpf/test_bpftool_synctypes.py   | 586 ++++++++++++++++++
>  29 files changed, 802 insertions(+), 96 deletions(-)
>  create mode 100755 tools/testing/selftests/bpf/test_bpftool_synctypes.py
>
> --
> 2.30.2
>

The patch set name ends abruptly at "synchronise and "... And what? I
need to know :)

Overall, it looks good, though I can't speak Python much, so I trust
the script works and we'll fix whatever is necessary as we go. I had
one small real nit about not re-formatting tons of existing lines for
no good reason, let's keep Git blame a bit more useful.

Also, it doesn't seem like you are actually calling a new script from
selftests/bpf/Makefile, right? That's good, because otherwise any UAPI
change in kernel header would require bpftool changes in the same
patch. But once this lands, we should probably run this in
kernel-patches CI ([0]) and, maybe, not sure, libbpf CI ([1]) as well.
So please follow up with that as well afterwards, that way you won't
be the only one nagging people about missed doc updates.

  [0] https://github.com/kernel-patches/vmtest/tree/master/travis-ci/vmtest
  [1] https://github.com/libbpf/libbpf/tree/master/travis-ci/vmtest