Messages in this thread | | | Date | Fri, 9 Sep 2022 10:06:27 -0700 | Subject | Re: [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin() | From | sdf@google ... |
| |
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
| |