On Fri, Nov 5, 2021 at 9:41 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote: > > 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. maybe --relaxed or just --legacy then? We can also have a warning or information message pointing to [0] for details of what is changing? And feel free to contribute with whatever important things that should be mentioned there as well. [0] https://github.com/libbpf/libbpf/wiki/Libbpf-1.0-migration-guide > > > 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!
