On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote: > > On Fri, Oct 22, 2021 at 10:05 AM John Fastabend > <john.fastabend@xxxxxxxxx> wrote: > > > > Stanislav Fomichev wrote: > > > Otherwise, attaching with bpftool doesn't work with strict section names. > > > > > > Also, switch to libbpf strict mode to use the latest conventions > > > (note, I don't think we have any cli api guarantees?). > > > > > > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx> > > > --- > > > tools/bpf/bpftool/main.c | 4 ++++ > > > tools/bpf/bpftool/prog.c | 15 +-------------- > > > 2 files changed, 5 insertions(+), 14 deletions(-) > > > > > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > > > index 02eaaf065f65..8223bac1e401 100644 > > > --- a/tools/bpf/bpftool/main.c > > > +++ b/tools/bpf/bpftool/main.c > > > @@ -409,6 +409,10 @@ int main(int argc, char **argv) > > > block_mount = false; > > > bin_name = argv[0]; > > > > > > + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL); > > > + if (ret) > > > + p_err("failed to enable libbpf strict mode: %d", ret); > > > + > > > > Would it better to just warn? Seems like this shouldn't be fatal from > > bpftool side? > > > > Also this is a potentially breaking change correct? Programs that _did_ > > work in the unstrict might suddently fail in the strict mode? If this > > is the case whats the versioning plan? We don't want to leak these > > type of changes across multiple versions, idealy we have a hard > > break and bump the version. > > > > I didn't catch a cover letter on the series. A small > > note about versioning and upgrading bpftool would be helpful. > > Yeah, it is a breaking change, every program that has non-strict > section names will be rejected. > > I mentioned that in the bpftool's commit description: > Also, switch to libbpf strict mode to use the latest conventions > (note, I don't think we have any cli api guarantees?). > > So I'm actually not sure what's the best way to handle this migration > and whether we really provide any cli guarantees to the users. I was > always assuming that bpftool is mostly for debugging/introspection, > but not sure. > > As Andrii suggested in another email, I can add a flag to disable this > strict mode. Any better ideas? Maybe the other way around for the transition period. Add a --strict flag to turn on libbpf strict mode? This follows libbpf's opt-in approach to breaking change. We can also emit warnings when people are trying to pin programs and mention that they should switch to --strict as in some future version this will be the default. Would that be better for users? > > > > > > > hash_init(prog_table.table); > > > hash_init(map_table.table); > > > hash_init(link_table.table); > > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > > > index 277d51c4c5d9..17505dc1243e 100644 > > > --- a/tools/bpf/bpftool/prog.c > > > +++ b/tools/bpf/bpftool/prog.c > > > @@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > > > > > while (argc) { > > > if (is_prefix(*argv, "type")) { > > > - char *type; > > > - > > > NEXT_ARG(); > > > > > > if (common_prog_type != BPF_PROG_TYPE_UNSPEC) { > > > @@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > > if (!REQ_ARGS(1)) > > > goto err_free_reuse_maps; > > > > > > - /* Put a '/' at the end of type to appease libbpf */ > > > - type = malloc(strlen(*argv) + 2); > > > - if (!type) { > > > - p_err("mem alloc failed"); > > > - goto err_free_reuse_maps; > > > - } > > > - *type = 0; > > > - strcat(type, *argv); > > > - strcat(type, "/"); > > > - > > > - err = get_prog_type_by_name(type, &common_prog_type, > > > + err = get_prog_type_by_name(*argv, &common_prog_type, > > > &expected_attach_type); > > > - free(type); > > > if (err < 0) > > > goto err_free_reuse_maps; > > > > This wont potentially break existing programs correct? It looks like > > just adding a '/' should be fine. > > > > Thanks, > > John