| Message ID | 1662702346-29665-2-git-send-email-wangyufen@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 16 of 16 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 127 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
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
在 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 >
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? :-) > > [...]
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)
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(-)