Re: [PATCH libbpf] libbpf: check if pin_path was set even map fd exist

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: "Hangbin Liu" <liuhangbin@gmail.com>, bpf <bpf@vger.kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Andrii Nakryiko" <andriin@fb.com>,
	Networking <netdev@vger.kernel.org>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>
Subject: Re: [PATCH libbpf] libbpf: check if pin_path was set even map fd exist
Date: Fri, 2 Oct 2020 11:13:41 -0700	[thread overview]
Message-ID: <CAEf4BzZBCYSdYxpAJ_gv0jRpuvTObpobQVccoqmsYVybgmfbxg@mail.gmail.com> (raw)
In-Reply-To: <20201002114936.GA20275@ranger.igk.intel.com>

On Fri, Oct 2, 2020 at 4:56 AM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Fri, Oct 02, 2020 at 03:57:50PM +0800, Hangbin Liu wrote:
> > Say a user reuse map fd after creating a map manually and set the
> > pin_path, then load the object via libbpf.
> >
> > In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will
> > return 0 if there is no pinned map in map->pin_path. Then after
> > checking if map fd exist, we should also check if pin_path was set
> > and do bpf_map__pin() instead of continue the loop.
> >
> > Fix it by creating map if fd not exist and continue checking pin_path
> > after that.
> >
> > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++---------------------
> >  1 file changed, 37 insertions(+), 38 deletions(-)
> >

[...]

> > +
> > +                     if (map->init_slots_sz) {
>
> Couldn't we flatten the code by inverting the logic here and using goto?

I explicitly asked Hangbin to not use goto to alter control flow here,
I'd like to keep goto within libbpf mostly for error handling.

>
>         if (!map->init_slot_sz) {
>                 pr_debug("map '%s': skipping creation (preset fd=%d)\n",
>                          map->name, map->fd);
>                 goto map_pin;
>         }
>
>         (...)
> map_pin:
>         if (map->pin_path && !map->pinned) {
>
> If I'm reading this right.
>

[...]

> > --
> > 2.25.4
> >