[bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin()

[bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin()

[Wang Yufen]

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

[bpf-next 2/2] libbpf: Add pathname_concat() helper

[Wang Yufen]

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

Re: [bpf-next 1/2] libbpf: Add make_path_and_pin() helper for bpf_xxx__pin()

[sdf]

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

Re: [bpf-next 2/2] libbpf: Add pathname_concat() helper

[sdf]

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

Re: [bpf-next 2/2] libbpf: Add pathname_concat() helper

[wangyufen]

在 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
>

Re: [bpf-next 2/2] libbpf: Add pathname_concat() helper

[Andrii Nakryiko]

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? :-)
>
>

[...]