Messages in this thread Patch in this message | | | Date | Fri, 9 Sep 2022 10:16:28 -0700 | Subject | Re: [bpf-next 2/2] libbpf: Add pathname_concat() helper | From | sdf@google ... |
| |
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
| |