* [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() @ 2022-09-09 5:45 Wang Yufen 2022-09-09 5:45 ` [bpf-next 2/2] libbpf: Add pathname_concat() helper Wang Yufen 2022-09-09 17:06 ` [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() sdf 0 siblings, 2 replies; 6+ messages in thread From: Wang Yufen @ 2022-09-09 5:45 UTC (permalink / raw) To: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa, paul.walmsley, palmer, aou, davem, kuba, hawk, nathan, ndesaulniers, trix Cc: bpf, linux-kernel, netdev, llvm Move make path, check path and pin to the common helper make_path_and_pin() to make the code simpler. Signed-off-by: Wang Yufen <wangyufen@huawei.com> --- tools/lib/bpf/libbpf.c | 56 ++++++++++++++++++++++---------------------------- 1 file changed, 24 insertions(+), 32 deletions(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 3ad1392..5854b92 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -7755,16 +7755,11 @@ static int check_path(const char *path) return err; } -int bpf_program__pin(struct bpf_program *prog, const char *path) +static int make_path_and_pin(int fd, const char *path) { char *cp, errmsg[STRERR_BUFSIZE]; int err; - if (prog->fd < 0) { - pr_warn("prog '%s': can't pin program that wasn't loaded\n", prog->name); - return libbpf_err(-EINVAL); - } - err = make_parent_dir(path); if (err) return libbpf_err(err); @@ -7773,12 +7768,27 @@ int bpf_program__pin(struct bpf_program *prog, const char *path) if (err) return libbpf_err(err); - if (bpf_obj_pin(prog->fd, path)) { + if (bpf_obj_pin(fd, path)) { err = -errno; cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg)); - pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, path, cp); + pr_warn("failed to pin at '%s': %s\n", path, cp); return libbpf_err(err); } + return 0; +} + +int bpf_program__pin(struct bpf_program *prog, const char *path) +{ + int err; + + if (prog->fd < 0) { + pr_warn("prog '%s': can't pin program that wasn't loaded\n", prog->name); + return libbpf_err(-EINVAL); + } + + err = make_path_and_pin(prog->fd, path); + if (err) + return libbpf_err(err); pr_debug("prog '%s': pinned at '%s'\n", prog->name, path); return 0; @@ -7838,32 +7848,20 @@ int bpf_map__pin(struct bpf_map *map, const char *path) map->pin_path = strdup(path); if (!map->pin_path) { err = -errno; - goto out_err; + cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg)); + pr_warn("failed to pin map: %s\n", cp); + return libbpf_err(err); } } - err = make_parent_dir(map->pin_path); - if (err) - return libbpf_err(err); - - err = check_path(map->pin_path); + err = make_path_and_pin(map->fd, map->pin_path); if (err) return libbpf_err(err); - if (bpf_obj_pin(map->fd, map->pin_path)) { - err = -errno; - goto out_err; - } - map->pinned = true; pr_debug("pinned map '%s'\n", map->pin_path); return 0; - -out_err: - cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg)); - pr_warn("failed to pin map: %s\n", cp); - return libbpf_err(err); } int bpf_map__unpin(struct bpf_map *map, const char *path) @@ -9611,19 +9609,13 @@ int bpf_link__pin(struct bpf_link *link, const char *path) if (link->pin_path) return libbpf_err(-EBUSY); - err = make_parent_dir(path); - if (err) - return libbpf_err(err); - err = check_path(path); - if (err) - return libbpf_err(err); link->pin_path = strdup(path); if (!link->pin_path) return libbpf_err(-ENOMEM); - if (bpf_obj_pin(link->fd, link->pin_path)) { - err = -errno; + err = make_path_and_pin(link->fd, path); + if (err) { zfree(&link->pin_path); return libbpf_err(err); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [bpf-next 2/2] libbpf: Add pathname_concat() helper 2022-09-09 5:45 [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() Wang Yufen @ 2022-09-09 5:45 ` Wang Yufen 2022-09-09 17:16 ` sdf 2022-09-09 17:06 ` [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() sdf 1 sibling, 1 reply; 6+ messages in thread From: Wang Yufen @ 2022-09-09 5:45 UTC (permalink / raw) To: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa, paul.walmsley, palmer, aou, davem, kuba, hawk, nathan, ndesaulniers, trix Cc: bpf, linux-kernel, netdev, llvm Move snprintf and len check to common helper pathname_concat() to make the code simpler. Signed-off-by: Wang Yufen <wangyufen@huawei.com> --- tools/lib/bpf/libbpf.c | 74 ++++++++++++++++++-------------------------------- 1 file changed, 27 insertions(+), 47 deletions(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 5854b92..238a03e 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -2096,20 +2096,31 @@ static bool get_map_field_int(const char *map_name, const struct btf *btf, return true; } -static int build_map_pin_path(struct bpf_map *map, const char *path) +static int pathname_concat(const char *path, const char *name, char *buf) { - char buf[PATH_MAX]; int len; - if (!path) - path = "/sys/fs/bpf"; - - len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); + len = snprintf(buf, PATH_MAX, "%s/%s", path, name); if (len < 0) return -EINVAL; else if (len >= PATH_MAX) return -ENAMETOOLONG; + return 0; +} + +static int build_map_pin_path(struct bpf_map *map, const char *path) +{ + char buf[PATH_MAX]; + int err; + + if (!path) + path = "/sys/fs/bpf"; + + err = pathname_concat(path, bpf_map__name(map), buf); + if (err) + return err; + return bpf_map__set_pin_path(map, buf); } @@ -7959,17 +7970,8 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) continue; if (path) { - int len; - - len = snprintf(buf, PATH_MAX, "%s/%s", path, - bpf_map__name(map)); - if (len < 0) { - err = -EINVAL; - goto err_unpin_maps; - } else if (len >= PATH_MAX) { - err = -ENAMETOOLONG; + if (pathname_concat(path, bpf_map__name(map), buf)) goto err_unpin_maps; - } sanitize_pin_path(buf); pin_path = buf; } else if (!map->pin_path) { @@ -8007,14 +8009,9 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) char buf[PATH_MAX]; if (path) { - int len; - - len = snprintf(buf, PATH_MAX, "%s/%s", path, - bpf_map__name(map)); - if (len < 0) - return libbpf_err(-EINVAL); - else if (len >= PATH_MAX) - return libbpf_err(-ENAMETOOLONG); + err = pathname_concat(path, bpf_map__name(map), buf); + if (err) + return err; sanitize_pin_path(buf); pin_path = buf; } else if (!map->pin_path) { @@ -8032,6 +8029,7 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) int bpf_object__pin_programs(struct bpf_object *obj, const char *path) { struct bpf_program *prog; + char buf[PATH_MAX]; int err; if (!obj) @@ -8043,17 +8041,8 @@ int bpf_object__pin_programs(struct bpf_object *obj, const char *path) } bpf_object__for_each_program(prog, obj) { - char buf[PATH_MAX]; - int len; - - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); - if (len < 0) { - err = -EINVAL; + if (pathname_concat(path, prog->name, buf)) goto err_unpin_programs; - } else if (len >= PATH_MAX) { - err = -ENAMETOOLONG; - goto err_unpin_programs; - } err = bpf_program__pin(prog, buf); if (err) @@ -8064,13 +8053,7 @@ int bpf_object__pin_programs(struct bpf_object *obj, const char *path) err_unpin_programs: while ((prog = bpf_object__prev_program(obj, prog))) { - char buf[PATH_MAX]; - int len; - - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); - if (len < 0) - continue; - else if (len >= PATH_MAX) + if (pathname_concat(path, prog->name, buf)) continue; bpf_program__unpin(prog, buf); @@ -8089,13 +8072,10 @@ int bpf_object__unpin_programs(struct bpf_object *obj, const char *path) bpf_object__for_each_program(prog, obj) { char buf[PATH_MAX]; - int len; - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); - if (len < 0) - return libbpf_err(-EINVAL); - else if (len >= PATH_MAX) - return libbpf_err(-ENAMETOOLONG); + err = pathname_concat(path, prog->name, buf); + if (err) + return libbpf_err(err); err = bpf_program__unpin(prog, buf); if (err) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [bpf-next 2/2] libbpf: Add pathname_concat() helper 2022-09-09 5:45 ` [bpf-next 2/2] libbpf: Add pathname_concat() helper Wang Yufen @ 2022-09-09 17:16 ` sdf 2022-09-13 2:55 ` wangyufen 2022-09-22 0:42 ` Andrii Nakryiko 0 siblings, 2 replies; 6+ messages in thread From: sdf @ 2022-09-09 17:16 UTC (permalink / raw) To: Wang Yufen Cc: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, haoluo, jolsa, paul.walmsley, palmer, aou, davem, kuba, hawk, nathan, ndesaulniers, trix, bpf, linux-kernel, netdev, llvm On 09/09, Wang Yufen wrote: > Move snprintf and len check to common helper pathname_concat() to make the > code simpler. > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > --- > tools/lib/bpf/libbpf.c | 74 > ++++++++++++++++++-------------------------------- > 1 file changed, 27 insertions(+), 47 deletions(-) > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 5854b92..238a03e 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -2096,20 +2096,31 @@ static bool get_map_field_int(const char > *map_name, const struct btf *btf, > return true; > } > -static int build_map_pin_path(struct bpf_map *map, const char *path) > +static int pathname_concat(const char *path, const char *name, char *buf) > { > - char buf[PATH_MAX]; > int len; > - if (!path) > - path = "/sys/fs/bpf"; > - > - len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); > + len = snprintf(buf, PATH_MAX, "%s/%s", path, name); > if (len < 0) > return -EINVAL; > else if (len >= PATH_MAX) > return -ENAMETOOLONG; > + return 0; > +} > + > +static int build_map_pin_path(struct bpf_map *map, const char *path) > +{ > + char buf[PATH_MAX]; > + int err; > + > + if (!path) > + path = "/sys/fs/bpf"; > + > + err = pathname_concat(path, bpf_map__name(map), buf); > + if (err) > + return err; > + > return bpf_map__set_pin_path(map, buf); > } > @@ -7959,17 +7970,8 @@ int bpf_object__pin_maps(struct bpf_object *obj, > const char *path) > continue; > if (path) { > - int len; > - > - len = snprintf(buf, PATH_MAX, "%s/%s", path, > - bpf_map__name(map)); > - if (len < 0) { > - err = -EINVAL; > - goto err_unpin_maps; > - } else if (len >= PATH_MAX) { > - err = -ENAMETOOLONG; [..] > + if (pathname_concat(path, bpf_map__name(map), buf)) > goto err_unpin_maps; > - } You're breaking error reporting here and in a bunch of other places. Should be: err = pathname_concat(); if (err) goto err_unpin_maps; I have the same attitude towards this patch as the first one in the series: not worth it. Nothing is currently broken, the code as is relatively readable, this version is not much simpler, it just looks slightly different taste-wise.. How about this: if you really want to push this kind of cleanup, send selftests that exercise all these error cases? :-) > sanitize_pin_path(buf); > pin_path = buf; > } else if (!map->pin_path) { > @@ -8007,14 +8009,9 @@ int bpf_object__unpin_maps(struct bpf_object *obj, > const char *path) > char buf[PATH_MAX]; > if (path) { > - int len; > - > - len = snprintf(buf, PATH_MAX, "%s/%s", path, > - bpf_map__name(map)); > - if (len < 0) > - return libbpf_err(-EINVAL); > - else if (len >= PATH_MAX) > - return libbpf_err(-ENAMETOOLONG); > + err = pathname_concat(path, bpf_map__name(map), buf); > + if (err) > + return err; > sanitize_pin_path(buf); > pin_path = buf; > } else if (!map->pin_path) { > @@ -8032,6 +8029,7 @@ int bpf_object__unpin_maps(struct bpf_object *obj, > const char *path) > int bpf_object__pin_programs(struct bpf_object *obj, const char *path) > { > struct bpf_program *prog; > + char buf[PATH_MAX]; > int err; > if (!obj) > @@ -8043,17 +8041,8 @@ int bpf_object__pin_programs(struct bpf_object > *obj, const char *path) > } > bpf_object__for_each_program(prog, obj) { > - char buf[PATH_MAX]; > - int len; > - > - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); > - if (len < 0) { > - err = -EINVAL; > + if (pathname_concat(path, prog->name, buf)) > goto err_unpin_programs; > - } else if (len >= PATH_MAX) { > - err = -ENAMETOOLONG; > - goto err_unpin_programs; > - } > err = bpf_program__pin(prog, buf); > if (err) > @@ -8064,13 +8053,7 @@ int bpf_object__pin_programs(struct bpf_object > *obj, const char *path) > err_unpin_programs: > while ((prog = bpf_object__prev_program(obj, prog))) { > - char buf[PATH_MAX]; > - int len; > - > - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); > - if (len < 0) > - continue; > - else if (len >= PATH_MAX) > + if (pathname_concat(path, prog->name, buf)) > continue; > bpf_program__unpin(prog, buf); > @@ -8089,13 +8072,10 @@ int bpf_object__unpin_programs(struct bpf_object > *obj, const char *path) > bpf_object__for_each_program(prog, obj) { > char buf[PATH_MAX]; > - int len; > - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); > - if (len < 0) > - return libbpf_err(-EINVAL); > - else if (len >= PATH_MAX) > - return libbpf_err(-ENAMETOOLONG); > + err = pathname_concat(path, prog->name, buf); > + if (err) > + return libbpf_err(err); > err = bpf_program__unpin(prog, buf); > if (err) > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bpf-next 2/2] libbpf: Add pathname_concat() helper 2022-09-09 17:16 ` sdf @ 2022-09-13 2:55 ` wangyufen 2022-09-22 0:42 ` Andrii Nakryiko 1 sibling, 0 replies; 6+ messages in thread From: wangyufen @ 2022-09-13 2:55 UTC (permalink / raw) To: sdf Cc: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, haoluo, jolsa, paul.walmsley, palmer, aou, davem, kuba, hawk, nathan, ndesaulniers, trix, bpf, linux-kernel, netdev, llvm 在 2022/9/10 1:16, sdf@google.com 写道: > On 09/09, Wang Yufen wrote: >> Move snprintf and len check to common helper pathname_concat() to >> make the >> code simpler. > >> Signed-off-by: Wang Yufen <wangyufen@huawei.com> >> --- >> tools/lib/bpf/libbpf.c | 74 >> ++++++++++++++++++-------------------------------- >> 1 file changed, 27 insertions(+), 47 deletions(-) > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 5854b92..238a03e 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -2096,20 +2096,31 @@ static bool get_map_field_int(const char >> *map_name, const struct btf *btf, >> return true; >> } > >> -static int build_map_pin_path(struct bpf_map *map, const char *path) >> +static int pathname_concat(const char *path, const char *name, char >> *buf) >> { >> - char buf[PATH_MAX]; >> int len; > >> - if (!path) >> - path = "/sys/fs/bpf"; >> - >> - len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); >> + len = snprintf(buf, PATH_MAX, "%s/%s", path, name); >> if (len < 0) >> return -EINVAL; >> else if (len >= PATH_MAX) >> return -ENAMETOOLONG; > >> + return 0; >> +} >> + >> +static int build_map_pin_path(struct bpf_map *map, const char *path) >> +{ >> + char buf[PATH_MAX]; >> + int err; >> + >> + if (!path) >> + path = "/sys/fs/bpf"; >> + >> + err = pathname_concat(path, bpf_map__name(map), buf); >> + if (err) >> + return err; >> + >> return bpf_map__set_pin_path(map, buf); >> } > >> @@ -7959,17 +7970,8 @@ int bpf_object__pin_maps(struct bpf_object >> *obj, const char *path) >> continue; > >> if (path) { >> - int len; >> - >> - len = snprintf(buf, PATH_MAX, "%s/%s", path, >> - bpf_map__name(map)); >> - if (len < 0) { >> - err = -EINVAL; >> - goto err_unpin_maps; >> - } else if (len >= PATH_MAX) { >> - err = -ENAMETOOLONG; > > [..] > >> + if (pathname_concat(path, bpf_map__name(map), buf)) >> goto err_unpin_maps; >> - } > > You're breaking error reporting here and in a bunch of other places. > Should be: > > err = pathname_concat(); > if (err) > goto err_unpin_maps; > Thanks for your comments. Sure, my bad. > I have the same attitude towards this patch as the first one in the > series: not worth it. Nothing is currently broken, the code as is > relatively > readable, this version is not much simpler, it just looks slightly > different > taste-wise.. > > How about this: if you really want to push this kind of cleanup, send > selftests that exercise all these error cases? :-) Ok. I will try. :-) > > >> sanitize_pin_path(buf); >> pin_path = buf; >> } else if (!map->pin_path) { >> @@ -8007,14 +8009,9 @@ int bpf_object__unpin_maps(struct bpf_object >> *obj, const char *path) >> char buf[PATH_MAX]; > >> if (path) { >> - int len; >> - >> - len = snprintf(buf, PATH_MAX, "%s/%s", path, >> - bpf_map__name(map)); >> - if (len < 0) >> - return libbpf_err(-EINVAL); >> - else if (len >= PATH_MAX) >> - return libbpf_err(-ENAMETOOLONG); >> + err = pathname_concat(path, bpf_map__name(map), buf); >> + if (err) >> + return err; >> sanitize_pin_path(buf); >> pin_path = buf; >> } else if (!map->pin_path) { >> @@ -8032,6 +8029,7 @@ int bpf_object__unpin_maps(struct bpf_object >> *obj, const char *path) >> int bpf_object__pin_programs(struct bpf_object *obj, const char *path) >> { >> struct bpf_program *prog; >> + char buf[PATH_MAX]; >> int err; > >> if (!obj) >> @@ -8043,17 +8041,8 @@ int bpf_object__pin_programs(struct bpf_object >> *obj, const char *path) >> } > >> bpf_object__for_each_program(prog, obj) { >> - char buf[PATH_MAX]; >> - int len; >> - >> - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); >> - if (len < 0) { >> - err = -EINVAL; >> + if (pathname_concat(path, prog->name, buf)) >> goto err_unpin_programs; >> - } else if (len >= PATH_MAX) { >> - err = -ENAMETOOLONG; >> - goto err_unpin_programs; >> - } > >> err = bpf_program__pin(prog, buf); >> if (err) >> @@ -8064,13 +8053,7 @@ int bpf_object__pin_programs(struct bpf_object >> *obj, const char *path) > >> err_unpin_programs: >> while ((prog = bpf_object__prev_program(obj, prog))) { >> - char buf[PATH_MAX]; >> - int len; >> - >> - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); >> - if (len < 0) >> - continue; >> - else if (len >= PATH_MAX) >> + if (pathname_concat(path, prog->name, buf)) >> continue; > >> bpf_program__unpin(prog, buf); >> @@ -8089,13 +8072,10 @@ int bpf_object__unpin_programs(struct >> bpf_object *obj, const char *path) > >> bpf_object__for_each_program(prog, obj) { >> char buf[PATH_MAX]; >> - int len; > >> - len = snprintf(buf, PATH_MAX, "%s/%s", path, prog->name); >> - if (len < 0) >> - return libbpf_err(-EINVAL); >> - else if (len >= PATH_MAX) >> - return libbpf_err(-ENAMETOOLONG); >> + err = pathname_concat(path, prog->name, buf); >> + if (err) >> + return libbpf_err(err); > >> err = bpf_program__unpin(prog, buf); >> if (err) >> -- >> 1.8.3.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bpf-next 2/2] libbpf: Add pathname_concat() helper 2022-09-09 17:16 ` sdf 2022-09-13 2:55 ` wangyufen @ 2022-09-22 0:42 ` Andrii Nakryiko 1 sibling, 0 replies; 6+ messages in thread From: Andrii Nakryiko @ 2022-09-22 0:42 UTC (permalink / raw) To: sdf Cc: Wang Yufen, andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, haoluo, jolsa, paul.walmsley, palmer, aou, davem, kuba, hawk, nathan, ndesaulniers, trix, bpf, linux-kernel, netdev, llvm On Fri, Sep 9, 2022 at 10:16 AM <sdf@google.com> wrote: > > On 09/09, Wang Yufen wrote: > > Move snprintf and len check to common helper pathname_concat() to make the > > code simpler. > > > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > > --- > > tools/lib/bpf/libbpf.c | 74 > > ++++++++++++++++++-------------------------------- > > 1 file changed, 27 insertions(+), 47 deletions(-) > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 5854b92..238a03e 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -2096,20 +2096,31 @@ static bool get_map_field_int(const char > > *map_name, const struct btf *btf, > > return true; > > } > > > -static int build_map_pin_path(struct bpf_map *map, const char *path) > > +static int pathname_concat(const char *path, const char *name, char *buf) > > { > > - char buf[PATH_MAX]; > > int len; > > > - if (!path) > > - path = "/sys/fs/bpf"; > > - > > - len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); > > + len = snprintf(buf, PATH_MAX, "%s/%s", path, name); > > if (len < 0) > > return -EINVAL; > > else if (len >= PATH_MAX) > > return -ENAMETOOLONG; > > > + return 0; > > +} > > + > > +static int build_map_pin_path(struct bpf_map *map, const char *path) > > +{ > > + char buf[PATH_MAX]; > > + int err; > > + > > + if (!path) > > + path = "/sys/fs/bpf"; > > + > > + err = pathname_concat(path, bpf_map__name(map), buf); > > + if (err) > > + return err; > > + > > return bpf_map__set_pin_path(map, buf); > > } > > > @@ -7959,17 +7970,8 @@ int bpf_object__pin_maps(struct bpf_object *obj, > > const char *path) > > continue; > > > if (path) { > > - int len; > > - > > - len = snprintf(buf, PATH_MAX, "%s/%s", path, > > - bpf_map__name(map)); > > - if (len < 0) { > > - err = -EINVAL; > > - goto err_unpin_maps; > > - } else if (len >= PATH_MAX) { > > - err = -ENAMETOOLONG; > > [..] > > > + if (pathname_concat(path, bpf_map__name(map), buf)) > > goto err_unpin_maps; > > - } > > You're breaking error reporting here and in a bunch of other places. > Should be: > > err = pathname_concat(); > if (err) > goto err_unpin_maps; > > I have the same attitude towards this patch as the first one in the > series: not worth it. Nothing is currently broken, the code as is relatively > readable, this version is not much simpler, it just looks slightly different > taste-wise.. > It's a minor improvement, IMO, so I don't mind it (5 repetitions of annoying error case handling seems worth streamlining). But selftests just for this seems like an overkill. > How about this: if you really want to push this kind of cleanup, send > selftests that exercise all these error cases? :-) > > [...] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() 2022-09-09 5:45 [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() Wang Yufen 2022-09-09 5:45 ` [bpf-next 2/2] libbpf: Add pathname_concat() helper Wang Yufen @ 2022-09-09 17:06 ` sdf 1 sibling, 0 replies; 6+ messages in thread From: sdf @ 2022-09-09 17:06 UTC (permalink / raw) To: Wang Yufen Cc: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, haoluo, jolsa, paul.walmsley, palmer, aou, davem, kuba, hawk, nathan, ndesaulniers, trix, bpf, linux-kernel, netdev, llvm On 09/09, Wang Yufen wrote: > Move make path, check path and pin to the common helper > make_path_and_pin() to make > the code simpler. Idk, I guess I'll defer to Andrii here; I won't really call it simpler. The code just looks different and looses some observability (see below). > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > --- > tools/lib/bpf/libbpf.c | 56 > ++++++++++++++++++++++---------------------------- > 1 file changed, 24 insertions(+), 32 deletions(-) > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 3ad1392..5854b92 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -7755,16 +7755,11 @@ static int check_path(const char *path) > return err; > } > -int bpf_program__pin(struct bpf_program *prog, const char *path) > +static int make_path_and_pin(int fd, const char *path) > { > char *cp, errmsg[STRERR_BUFSIZE]; > int err; > - if (prog->fd < 0) { > - pr_warn("prog '%s': can't pin program that wasn't loaded\n", > prog->name); > - return libbpf_err(-EINVAL); > - } > - > err = make_parent_dir(path); > if (err) > return libbpf_err(err); > @@ -7773,12 +7768,27 @@ int bpf_program__pin(struct bpf_program *prog, > const char *path) > if (err) > return libbpf_err(err); > - if (bpf_obj_pin(prog->fd, path)) { > + if (bpf_obj_pin(fd, path)) { > err = -errno; > cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg)); > - pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, path, > cp); > + pr_warn("failed to pin at '%s': %s\n", path, cp); This seems to be making logging worse? We went from "can't pin 'program name' to 'path': ENOENT" to "can't pin to 'path': ENOENT". You can deduce the program name from the path, but why not keep the proper log message? > return libbpf_err(err); > } > + return 0; > +} > + > +int bpf_program__pin(struct bpf_program *prog, const char *path) > +{ > + int err; > + > + if (prog->fd < 0) { > + pr_warn("prog '%s': can't pin program that wasn't loaded\n", > prog->name); > + return libbpf_err(-EINVAL); > + } > + > + err = make_path_and_pin(prog->fd, path); > + if (err) > + return libbpf_err(err); > pr_debug("prog '%s': pinned at '%s'\n", prog->name, path); > return 0; > @@ -7838,32 +7848,20 @@ int bpf_map__pin(struct bpf_map *map, const char > *path) > map->pin_path = strdup(path); > if (!map->pin_path) { > err = -errno; > - goto out_err; > + cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg)); > + pr_warn("failed to pin map: %s\n", cp); > + return libbpf_err(err); > } > } > - err = make_parent_dir(map->pin_path); > - if (err) > - return libbpf_err(err); > - > - err = check_path(map->pin_path); > + err = make_path_and_pin(map->fd, map->pin_path); > if (err) > return libbpf_err(err); > - if (bpf_obj_pin(map->fd, map->pin_path)) { > - err = -errno; > - goto out_err; > - } > - > map->pinned = true; > pr_debug("pinned map '%s'\n", map->pin_path); > return 0; > - > -out_err: > - cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg)); > - pr_warn("failed to pin map: %s\n", cp); > - return libbpf_err(err); > } > int bpf_map__unpin(struct bpf_map *map, const char *path) > @@ -9611,19 +9609,13 @@ int bpf_link__pin(struct bpf_link *link, const > char *path) > if (link->pin_path) > return libbpf_err(-EBUSY); > - err = make_parent_dir(path); > - if (err) > - return libbpf_err(err); > - err = check_path(path); > - if (err) > - return libbpf_err(err); > link->pin_path = strdup(path); > if (!link->pin_path) > return libbpf_err(-ENOMEM); > - if (bpf_obj_pin(link->fd, link->pin_path)) { > - err = -errno; > + err = make_path_and_pin(link->fd, path); > + if (err) { > zfree(&link->pin_path); > return libbpf_err(err); > } > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-22 0:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-09 5:45 [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() Wang Yufen 2022-09-09 5:45 ` [bpf-next 2/2] libbpf: Add pathname_concat() helper Wang Yufen 2022-09-09 17:16 ` sdf 2022-09-13 2:55 ` wangyufen 2022-09-22 0:42 ` Andrii Nakryiko 2022-09-09 17:06 ` [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() sdf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.