netdev - Re: [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Feb 2019 18:47:17 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define
 for map iteration

On Wed, Feb 27, 2019 at 03:57:03PM -0800, Jakub Kicinski wrote:
> On Wed, 27 Feb 2019 15:47:56 -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
> > <jakub.kicinski@...ronome.com> wrote:
> > >
> > > For historical reasons the helper to loop over maps in an object
> > > is called bpf_map__for_each while it really should be called
> > > bpf_object__for_each_map.  Rename and add a correctly named
> > > define for backward compatibility.  
> > 
> > Seems like there are at least 3 more functions that are not named correctly:
> > - __bpf_map__iter (__bpf_object__iter_map?)
> > - bpf_map__next (=> bpf_object__next_map?)
> > - bpf_map__prev (=> bpf_object__prev_map?)
> > 
> > Let's rename them as well?
> 
> At least those are consistently named between programs and maps.

I think this patch makes naming consistent enough.

> I'm happy to do the rename if we don't need backward compat, seems 
> a little much to add aliases?
> 
> > > Switch all in-tree users to the correct name (Quentin).
> > >
> > > Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> > > Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.com>
> 
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 6c0168f8bba5..b4652aa1a58a 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
> > >
> > >  LIBBPF_API struct bpf_map *
> > >  bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
> > > -#define bpf_map__for_each(pos, obj)            \
> > > +#define bpf_object__for_each_map(pos, obj)             \
> > >         for ((pos) = bpf_map__next(NULL, (obj));        \
> > >              (pos) != NULL;                             \
> > >              (pos) = bpf_map__next((pos), (obj)))
> > > +#define bpf_map__for_each bpf_object__for_each_map  
> > 
> > Should we get rid of this as well, instead of accumulating cruft?
> 
> Well, we did some gymnastics in the past to maintain backward compat, 
> I thought we do need it..?

We do need to keep backward compat.
This line is necessary.

imo this set looks good to me as-is.