On Fri, Nov 5, 2021 at 4:02 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote: > > 2021-11-04 09:03 UTC-0700 ~ Stanislav Fomichev <sdf@xxxxxxxxxx> > > Otherwise, attaching with bpftool doesn't work with strict section names. > > > > Also: > > > > - by default, don't append / to the section name; in strict > > mode it's relevant only for a small subset of prog types > > - print a deprecation warning when requested to pin all programs > > > > + bpftool prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe > > Warning: pinning by section name is deprecated, use --strict to pin by function name. > > See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences > > > > + bpftool prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp > > Warning: pinning by section name is deprecated, use --strict to pin by function name. > > See: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences > > > > + bpftool --strict prog loadall tools/testing/selftests/bpf/test_probe_user.o /sys/fs/bpf/kprobe type kprobe > > + bpftool --strict prog loadall tools/testing/selftests/bpf/xdp_dummy.o /sys/fs/bpf/xdp type xdp > > > > Cc: Quentin Monnet <quentin@xxxxxxxxxxxxx> > > Cc: John Fastabend <john.fastabend@xxxxxxxxx> > > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx> > Hi and thanks Stanislav! I have some reservations on the current > approach, though. > > I see the new option is here to avoid breaking the current behaviour. > However: > > - Libbpf has the API break scheduled for v1.0, and at this stage we > won't be able to avoid breakage in bpftool's behaviour. This means that, > eventually, "bpftool prog loadall" will load functions by func name and > not section name, that section names with garbage prefixes > ('SEC("xdp_my_prog")') will be rejected, and that maps with extra > attributes in their definitions will be rejected. And save for the > pinning path difference, we won't be able to tell from bpftool when this > happens, because this is all handled by libbpf. > > - In that context, I'd rather have the strict behaviour being the > default. We could have an option to restore the legacy behaviour > (disabling strict mode) during the transition, but I'd rather avoid > users starting to use everywhere a "--strict" option which becomes > either mandatory in the future or (more likely) a deprecated no-op when > we switch to libbpf v1.0 and break legacy behaviour anyway. > > - If we were to keep the current option, I'm not a fan of the "--strict" > name, because from a user point of view, I don't think it reflects well > the change to pinning by function name, for example. But given that the > option interferes with different aspects of the loading process, I don't > really have a better suggestion :/. Yeah, not sure what's the best way here. Strict by default will break users, but at least we can expect some action. Non-strict by default will most likely not cause anybody to add --strict or react to that warning. I agree with your point though regarding --strict being default at some point and polluting every bpftool call with it doesn't look good, so I'll try to switch to strict by default in v2. RE naming: we can use --legacy-libbpf instead, but it also doesn't really tell what the real changes are. > Aside from the discussion on this option, the code looks good. The > optional '/' on program types on the command line works well, thanks for > preserving the behaviour on the CLI. Please find also a few more notes > below. > > > --- > > .../bpftool/Documentation/common_options.rst | 6 +++ > > tools/bpf/bpftool/main.c | 13 +++++- > > tools/bpf/bpftool/main.h | 1 + > > tools/bpf/bpftool/prog.c | 40 +++++++++++-------- > > 4 files changed, 43 insertions(+), 17 deletions(-) > > > > diff --git a/tools/bpf/bpftool/Documentation/common_options.rst b/tools/bpf/bpftool/Documentation/common_options.rst > > index 05d06c74dcaa..28710f9005be 100644 > > --- a/tools/bpf/bpftool/Documentation/common_options.rst > > +++ b/tools/bpf/bpftool/Documentation/common_options.rst > > @@ -20,3 +20,9 @@ > > Print all logs available, even debug-level information. This includes > > logs from libbpf as well as from the verifier, when attempting to > > load programs. > > + > > +-S, --strict > > The option does not affect all commands, and could maybe be moved to the > pages of the relevant commands. I think that "prog" and "map" are affected? True, but probably still a good idea to exercise that strict mode everywhere in the bpftool itself? To make sure other changes don't break it in a significant way. > > + Use strict (aka v1.0) libbpf mode which has more stringent section > > + name requirements. > > + See https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#pinning-path-differences > > There is more than just the pinning difference. The stricter section > name handling and the changes for map declarations (drop of non-BTF and > of unknown attributes) should also affect the way bpftool can load > objects. Even the changes in the ELF section processing might affect the > resulting objects. Ack. Will add a better description here to point to the overall list of libbpf-1.0 changes. > > + for details. > Note that if we add a command line option, we'd also need to add it to > the interactive help message and bash completion: > > https://elixir.bootlin.com/linux/v5.15/source/tools/bpf/bpftool/main.h#L57 > https://elixir.bootlin.com/linux/v5.15/source/tools/bpf/bpftool/bash-completion/bpftool#L263 Thanks, will do!
