Re: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

[Stanislav Fomichev <sdf@google.com>]

On Fri, Oct 22, 2021 at 10:05 AM John Fastabend
<john.fastabend@gmail.com> 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@google.com>
> > ---
> >  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?




> >       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