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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Feb 27, 2019 at 6:47 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> 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@netronome.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@netronome.com>
> > > > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.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.
>

bpf_map__next/prev and bpf_program__next/prev convention feels a bit
weird, they fill more like methods of bpf_object rather than methods
of bpf_map and bpf_program, but I can understand why we might want to
preserve them.

Would it make sense to still add bpf_object__{next,prev}_{map,program}
(with struct bpf_object as a first arg) and just call them from
bpf_{map,program}__{next,prev}? That way we'll have consistent naming
that follows libbpf's own naming guidelines and we'll preserve
backwards compatibility? We can do that in follow up patches, if at
all, so I'll ack this patch.


Acked-by: Andrii Nakryiko <andriin@fb.com>