On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> In order to show pinned paths for BPF programs, maps, or links when
> listing them with the "-f" option, bpftool creates hash maps to store
> all relevant paths under the bpffs. So far, it would rely on the
> kernel implementation (from tools/include/linux/hashtable.h).
>
> We can make bpftool rely on libbpf's implementation instead. The
> motivation is to make bpftool less dependent of kernel headers, to ease
> the path to a potential out-of-tree mirror, like libbpf has.
>
> This commit is the first step of the conversion: the hash maps for
> pinned paths for programs, maps, and links are converted to libbpf's
> hashmap.{c,h}. Other hash maps used for the PIDs of process holding
> references to BPF objects are left unchanged for now. On the build side,
> this requires adding a dependency to a second header internal to libbpf,
> and making it a dependency for the bootstrap bpftool version as well.
> The rest of the changes are a rather straightforward conversion.
>
> Signed-off-by: Quentin Monnet <quentin@xxxxxxxxxxxxx>
> ---
> tools/bpf/bpftool/Makefile | 8 +++---
> tools/bpf/bpftool/common.c | 50 ++++++++++++++++++++------------------
> tools/bpf/bpftool/link.c | 35 ++++++++++++++------------
> tools/bpf/bpftool/main.h | 29 +++++++++++++---------
> tools/bpf/bpftool/map.c | 35 ++++++++++++++------------
> tools/bpf/bpftool/prog.c | 35 ++++++++++++++------------
> 6 files changed, 105 insertions(+), 87 deletions(-)
>
[...]
> @@ -420,28 +421,20 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb,
> if (bpf_obj_get_info_by_fd(fd, &pinned_info, &len))
> goto out_close;
>
> - obj_node = calloc(1, sizeof(*obj_node));
> - if (!obj_node) {
> + path = strdup(fpath);
> + if (!path) {
> err = -1;
> goto out_close;
> }
>
> - obj_node->id = pinned_info.id;
> - obj_node->path = strdup(fpath);
> - if (!obj_node->path) {
> - err = -1;
> - free(obj_node);
> - goto out_close;
> - }
> -
> - hash_add(build_fn_table->table, &obj_node->hash, obj_node->id);
> + hashmap__append(build_fn_table, u32_as_hash_field(pinned_info.id), path);
handle errors? operation can fail
> out_close:
> close(fd);
> out_ret:
> return err;
> }
[...]
>
> unsigned int get_page_size(void)
> @@ -962,3 +956,13 @@ int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len)
>
> return fd;
> }
> +
> +size_t bpftool_hash_fn(const void *key, void *ctx)
> +{
> + return (size_t)key;
> +}
> +
> +bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx)
kind of too generic and too assuming function (hash_fn and
equal_fn)... Maybe either use static functions near each hashmap use
case, or name it to specify that it works when keys are ids?
> +{
> + return k1 == k2;
> +}
[...]
> @@ -256,4 +247,18 @@ int do_filter_dump(struct tcmsg *ifinfo, struct nlattr **tb, const char *kind,
>
> int print_all_levels(__maybe_unused enum libbpf_print_level level,
> const char *format, va_list args);
> +
> +size_t bpftool_hash_fn(const void *key, void *ctx);
> +bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx);
> +
> +static inline void *u32_as_hash_field(__u32 x)
it's used for keys only, right? so u32_as_hash_key?
> +{
> + return (void *)(uintptr_t)x;
> +}
> +
> +static inline bool hashmap__empty(struct hashmap *map)
> +{
> + return map ? hashmap__size(map) == 0 : true;
> +}
> +
> #endif
[...]
