Re: [PATCH bpf-next v4 2/5] libbpf: Store map pin path and status in struct bpf_map

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

On Tue, Oct 29, 2019 at 12:39 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Support storing and setting a pin path in struct bpf_map, which can be used
> for automatic pinning. Also store the pin status so we can avoid attempts
> to re-pin a map that has already been pinned (or reused from a previous
> pinning).
>
> The behaviour of bpf_object__{un,}pin_maps() is changed so that if it is
> called with a NULL path argument (which was previously illegal), it will
> (un)pin only those maps that have a pin_path set.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Looks good, thanks! Just some minor things to fix up below.

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

>  tools/lib/bpf/libbpf.c   |  164 +++++++++++++++++++++++++++++++++++-----------
>  tools/lib/bpf/libbpf.h   |    8 ++
>  tools/lib/bpf/libbpf.map |    3 +
>  3 files changed, 134 insertions(+), 41 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ce5ef3ddd263..fd11f6aeb32c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -226,6 +226,8 @@ struct bpf_map {
>         void *priv;
>         bpf_map_clear_priv_t clear_priv;
>         enum libbpf_map_type libbpf_type;
> +       char *pin_path;
> +       bool pinned;
>  };
>
>  struct bpf_secdata {
> @@ -4025,47 +4027,119 @@ int bpf_map__pin(struct bpf_map *map, const char *path)
>         char *cp, errmsg[STRERR_BUFSIZE];
>         int err;
>
> -       err = check_path(path);
> -       if (err)
> -               return err;
> -
>         if (map == NULL) {
>                 pr_warn("invalid map pointer\n");
>                 return -EINVAL;
>         }
>
> -       if (bpf_obj_pin(map->fd, path)) {
> -               cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
> -               pr_warn("failed to pin map: %s\n", cp);
> -               return -errno;
> +       if (map->pin_path) {
> +               if (path && strcmp(path, map->pin_path)) {
> +                       pr_warn("map '%s' already has pin path '%s' different from '%s'\n",
> +                               bpf_map__name(map), map->pin_path, path);
> +                       return -EINVAL;
> +               } else if (map->pinned) {
> +                       pr_debug("map '%s' already pinned at '%s'; not re-pinning\n",
> +                                bpf_map__name(map), map->pin_path);
> +                       return 0;
> +               }

`if (map->pinned)` check is the same in both branches, so I'd do it
first, before this map->pin_path if/else.

> +       } else {
> +               if (!path) {
> +                       pr_warn("missing a path to pin map '%s' at\n",
> +                               bpf_map__name(map));
> +                       return -EINVAL;
> +               } else if (map->pinned) {
> +                       pr_warn("map '%s' already pinned\n", bpf_map__name(map));
> +                       return -EEXIST;
> +               }
> +
> +               map->pin_path = strdup(path);
> +               if (!map->pin_path) {
> +                       err = -errno;
> +                       goto out_err;
> +               }
>         }
>

[...]

> +
> +       err = check_path(path);
> +       if (err)
> +               return err;
> +
>         err = unlink(path);
>         if (err != 0)
>                 return -errno;
> -       pr_debug("unpinned map '%s'\n", path);
> +
> +       map->pinned = false;
> +       pr_debug("unpinned map from '%s' from '%s'\n", bpf_map__name(map), path);

typo: extra from before map name?

>
>         return 0;
>  }
>

[...]

>
>         return err;
> @@ -4131,17 +4205,24 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
>                 return -ENOENT;
>
>         bpf_object__for_each_map(map, obj) {
> +               char *pin_path = NULL;
>                 char buf[PATH_MAX];

you can call buf as pin_path and get rid of extra pointer?

> -               int len;
>
> -               len = snprintf(buf, PATH_MAX, "%s/%s", path,
> -                              bpf_map__name(map));
> -               if (len < 0)
> -                       return -EINVAL;
> -               else if (len >= PATH_MAX)
> -                       return -ENAMETOOLONG;
> +               if (path) {
> +                       int len;
> +
> +                       len = snprintf(buf, PATH_MAX, "%s/%s", path,
> +                                      bpf_map__name(map));
> +                       if (len < 0)
> +                               return -EINVAL;
> +                       else if (len >= PATH_MAX)
> +                               return -ENAMETOOLONG;
> +                       pin_path = buf;
> +               } else if (!map->pin_path) {
> +                       continue;
> +               }
>
> -               err = bpf_map__unpin(map, buf);
> +               err = bpf_map__unpin(map, pin_path);
>                 if (err)
>                         return err;
>         }

[...]

> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index d1473ea4d7a5..c24d4c01591d 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -197,4 +197,7 @@ LIBBPF_0.0.6 {
>                 bpf_object__open_mem;
>                 bpf_program__get_expected_attach_type;
>                 bpf_program__get_type;
> +               bpf_map__get_pin_path;
> +               bpf_map__set_pin_path;
> +               bpf_map__is_pinned;

we try to keep this list alphabetically sorted

>  } LIBBPF_0.0.5;
>