Re: [PATCH bpf-next v3 2/4] libbpf: Store map pin path and status in struct bpf_map
From: Andrii Nakryiko <hidden>
Date: 2019-10-28 18:24:32
Also in:
bpf
On Sun, Oct 27, 2019 at 1:53 PM Toke Høiland-Jørgensen [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Toke Høiland-Jørgensen <redacted> 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). Acked-by: Andrii Nakryiko <redacted> Signed-off-by: Toke Høiland-Jørgensen <redacted> --- tools/lib/bpf/libbpf.c | 115 ++++++++++++++++++++++++++++++++++++---------- tools/lib/bpf/libbpf.h | 3 + tools/lib/bpf/libbpf.map | 3 + 3 files changed, 97 insertions(+), 24 deletions(-)diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index ce5ef3ddd263..eb1c5e6ad4a3 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,118 @@ 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->pinned) { + pr_warn("map already pinned\n");
it would be helpful to print the name of the map, otherwise user will have to guess
+ return -EEXIST;
+ }
+
+ if (path && map->pin_path && strcmp(path, map->pin_path)) {
+ pr_warn("map already has pin path '%s' different from '%s'\n",
+ map->pin_path, path);here pin_path probably would be unique enough, but for consistency we might want to print map name as well
+ return -EINVAL;
+ }
+
+ if (!map->pin_path && !path) {
+ pr_warn("missing pin path\n");and here?
+ return -EINVAL;
}
- pr_debug("pinned map '%s'\n", path);
+ if (!map->pin_path) {
+ map->pin_path = strdup(path);
+ if (!map->pin_path) {
+ err = -errno;
+ goto out_err;
+ }
+ }
There is a bit of repetition of if conditions, based on whether we
have map->pin_path set (which is the most critical piece we care
about), so that makes it a bit harder to follow what's going on. How
about this structure, would it make a bit clearer what the error
conditions are? Not insisting, though.
if (map->pin_path) {
if (path && strcmp(...))
bad, exit
else { /* no pin_path */
if (!path)
very bad, exit
map->pin_path = strdup(..)
if (!map->pin_path)
also bad, exit
}
+ + err = check_path(map->pin_path); + if (err) + return err; +
[...]
+int bpf_map__set_pin_path(struct bpf_map *map, const char *path)
+{
+ char *old = map->pin_path, *new;
+
+ if (path) {
+ new = strdup(path);
+ if (!new)
+ return -errno;
+ } else {
+ new = NULL;
+ }
+
+ map->pin_path = new;
+ if (old)
+ free(old);you don't really need old, just free map->pin_path before setting it to new. Also assigning new = NULL will simplify if above.
quoted hunk ↗ jump to hunk
+ + return 0; +} + +const char *bpf_map__get_pin_path(struct bpf_map *map) +{ + return map->pin_path; +} + +bool bpf_map__is_pinned(struct bpf_map *map) +{ + return map->pinned; +} + int bpf_object__pin_maps(struct bpf_object *obj, const char *path) { struct bpf_map *map;@@ -4106,17 +4179,10 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
I might have missed something the change in some other patch, but shouldn't pin_maps ignore already pinned maps? Otherwise we'll be generating unnecessary warnings?
err_unpin_maps:
while ((map = bpf_map__prev(map, obj))) {
- char buf[PATH_MAX];
- int len;
-
- len = snprintf(buf, PATH_MAX, "%s/%s", path,
- bpf_map__name(map));
- if (len < 0)
- continue;
- else if (len >= PATH_MAX)
+ if (!map->pin_path)
continue;
- bpf_map__unpin(map, buf);
+ bpf_map__unpin(map, NULL);so this will unpin auto-pinned maps (from BTF-defined maps). Is that the desired behavior? I guess it might be ok (if you can't pin all of your maps, you should probably clean all of them up?), but just bringing it up.
quoted hunk ↗ jump to hunk
} return err;@@ -4266,6 +4332,7 @@ void bpf_object__close(struct bpf_object *obj) for (i = 0; i < obj->nr_maps; i++) { zfree(&obj->maps[i].name); + zfree(&obj->maps[i].pin_path); if (obj->maps[i].clear_priv) obj->maps[i].clear_priv(&obj->maps[i], obj->maps[i].priv);diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index c63e2ff84abc..a514729c43f5 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h@@ -385,6 +385,9 @@ LIBBPF_API int bpf_map__resize(struct bpf_map *map, __u32 max_entries); LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map); LIBBPF_API bool bpf_map__is_internal(const struct bpf_map *map); LIBBPF_API void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex); +LIBBPF_API int bpf_map__set_pin_path(struct bpf_map *map, const char *path); +LIBBPF_API const char *bpf_map__get_pin_path(struct bpf_map *map); +LIBBPF_API bool bpf_map__is_pinned(struct bpf_map *map); LIBBPF_API int bpf_map__pin(struct bpf_map *map, const char *path); LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);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; } LIBBPF_0.0.5;