| Message ID | 1662702346-29665-1-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 | warning | WARNING: line length of 89 exceeds 80 columns |
| 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 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
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); }
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(-)