* [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 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
* 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
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.