Re: [PATCH] bpftool: add support of pin prog by name

From: Menglong Dong
Date: Mon Dec 06 2021 - 20:58:11 EST


On Tue, Dec 7, 2021 at 5:22 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Sat, Dec 4, 2021 at 8:51 PM <menglong8.dong@xxxxxxxxx> wrote:
> >
> > From: Menglong Dong <imagedong@xxxxxxxxxxx>
> >
> > For now, the command 'bpftool prog loadall' use section name as the
> > name of the pin file. However, once there are prog with the same
> > section name in ELF file, this command will failed with the error
> > 'File Exist'.
> >
> > So, add the support of pin prog by function name with the 'pinbyname'
> > argument.
> >
> > Signed-off-by: Menglong Dong <imagedong@xxxxxxxxxxx>
> > ---
>
> Doesn't [0] do that already?
>
> [0] https://patchwork.kernel.org/project/netdevbpf/patch/20211021214814.1236114-2-sdf@xxxxxxxxxx/
>

Ops....Sorry, I didn't notice that patch :/

> > tools/bpf/bpftool/prog.c | 7 +++++++
> > tools/lib/bpf/libbpf.c | 5 +++++
> > tools/lib/bpf/libbpf.h | 2 ++
> > 3 files changed, 14 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index e47e8b06cc3d..74e0aaebfefc 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -1471,6 +1471,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> > unsigned int old_map_fds = 0;
> > const char *pinmaps = NULL;
> > struct bpf_object *obj;
> > + bool pinbyname = false;
> > struct bpf_map *map;
> > const char *pinfile;
> > unsigned int i, j;
> > @@ -1589,6 +1590,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> > goto err_free_reuse_maps;
> >
> > pinmaps = GET_ARG();
> > + } else if (is_prefix(*argv, "pinbyname")) {
> > + pinbyname = true;
> > + NEXT_ARG();
> > } else {
> > p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?",
> > *argv);
> > @@ -1616,6 +1620,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> > goto err_close_obj;
> > }
> >
> > + if (pinbyname)
> > + bpf_program__set_pinname(pos,
> > + (char *)bpf_program__name(pos));
> > bpf_program__set_ifindex(pos, ifindex);
> > bpf_program__set_type(pos, prog_type);
> > bpf_program__set_expected_attach_type(pos, expected_attach_type);
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index f6faa33c80fa..e8fc1d0fe16e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8119,6 +8119,11 @@ void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex)
> > prog->prog_ifindex = ifindex;
> > }
> >
> > +void bpf_program__set_pinname(struct bpf_program *prog, char *name)
> > +{
> > + prog->pin_name = name;
>
> BPF maps have bpf_map__set_pin_path(), setting a full path is more
> flexible approach, I think, so if we had to do something here, it's
> better to add bpf_program__set_ping_path().

Yeah, I think it's a good idea. I'll do something about it.

Thanks!
Menglong Dong

>
>
> > +}
> > +
> > const char *bpf_program__name(const struct bpf_program *prog)
> > {
> > return prog->name;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 4ec69f224342..107cf736c2bb 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -216,6 +216,8 @@ LIBBPF_API int bpf_program__set_priv(struct bpf_program *prog, void *priv,
> > LIBBPF_API void *bpf_program__priv(const struct bpf_program *prog);
> > LIBBPF_API void bpf_program__set_ifindex(struct bpf_program *prog,
> > __u32 ifindex);
> > +LIBBPF_API void bpf_program__set_pinname(struct bpf_program *prog,
> > + char *name);
> >
> > LIBBPF_API const char *bpf_program__name(const struct bpf_program *prog);
> > LIBBPF_API const char *bpf_program__section_name(const struct bpf_program *prog);
> > --
> > 2.30.2
> >