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

On Wed, Feb 27, 2019 at 6:47 PM Alexei Starovoitov
[off-list ref] wrote:

On Wed, Feb 27, 2019 at 03:57:03PM -0800, Jakub Kicinski wrote:

quoted

On Wed, 27 Feb 2019 15:47:56 -0800, Andrii Nakryiko wrote:

quoted

On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
[off-list ref] wrote:

quoted

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.

quoted

I'm happy to do the rename if we don't need backward compat, seems
a little much to add aliases?

quoted

quoted

Switch all in-tree users to the correct name (Quentin).

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Quentin Monnet <redacted>

quoted

quoted

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