Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
> On Sun, Oct 27, 2019 at 1:53 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>>
>> 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 <andriin@xxxxxx>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>> ---
>> 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
Well, the existing error message didn't include the map name, so I was
just being consistent. But sure I can change it (and the old message as
well).
>> + 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
> }
Hmm, yeah, this may be better...
>> +
>> + 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.
Right, will fix.
>> +
>> + 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?
Well, in the previous version this was in one of the options you didn't
like. If I just change pin_maps() unconditionally, that will be a change
in behaviour in an existing API. So I figured it would be better to
leave this as-is. I don't think this function is really useful along
with the auto-pinning anyway. If you're pinning all maps, why use
auto-pinning? And if you want to do something custom to all the
non-pinned maps you'd probably iterate through them yourself anyway and
can react appropriately?
>>
>> 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.
Yeah, I realise that. Not entirely sure it's the right thing to do, but
there not really any way to disambiguate how the map was pinned; unless
we want to add another state field just for that? So I guess it's either
"don't do any cleanup" or just "unpin everything". And since I don't
think it'll be terribly useful to combine the use of this function with
auto-pinning anyway, I think it's probably fine to just unpin everything
here?
-Toke
