[PATCH bpf-next v2] bpftool: Mount bpffs on provided dir instead of parent dir

[PATCH bpf-next v2] bpftool: Mount bpffs on provided dir instead of parent dir

[Sahil Siddiq]

When pinning programs/objects under PATH (eg: during "bpftool prog
loadall") the bpffs is mounted on the parent dir of PATH in the
following situations:
- the given dir exists but it is not bpffs.
- the given dir doesn't exist and the parent dir is not bpffs.

Mounting on the parent dir can also have the unintentional side-
effect of hiding other files located under the parent dir.

If the given dir exists but is not bpffs, then the bpffs should
be mounted on the given dir and not its parent dir.

Similarly, if the given dir doesn't exist and its parent dir is not
bpffs, then the given dir should be created and the bpffs should be
mounted on this new dir.

Link: https://lore.kernel.org/bpf/2da44d24-74ae-a564-1764-afccf395eeec@isovalent.com/T/#t

Closes: https://github.com/libbpf/bpftool/issues/100

Fixes: 2a36c26fe3b8 ("bpftool: Support bpffs mountpoint as pin path for prog loadall")

Signed-off-by: Sahil Siddiq <icegambit91@gmail.com>
---
Changes since v1:
 - Split "mount_bpffs_for_pin" into two functions:
   - mount_bpffs_on_dir
   - mount_bpffs_given_file
   This is done to improve maintainability and readability.
 - Improve error messages
 - (mount_bpffs_given_file): If the file already exists, throw an
   error instead of waiting for bpf_obj_pin() to throw an error
 - Code style changes

 tools/bpf/bpftool/common.c     | 78 +++++++++++++++++++++++++++++-----
 tools/bpf/bpftool/iter.c       |  2 +-
 tools/bpf/bpftool/main.h       |  3 +-
 tools/bpf/bpftool/prog.c       |  5 ++-
 tools/bpf/bpftool/struct_ops.c |  2 +-
 5 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index cc6e6aae2447..d2746681200e 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -244,24 +244,80 @@ int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type)
 	return fd;
 }
 
-int mount_bpffs_for_pin(const char *name, bool is_dir)
+int mount_bpffs_on_dir(const char *dir_name)
 {
 	char err_str[ERR_MAX_LEN];
-	char *file;
-	char *dir;
 	int err = 0;
 
-	if (is_dir && is_bpffs(name))
+	if (is_bpffs(dir_name))
 		return err;
 
-	file = malloc(strlen(name) + 1);
-	if (!file) {
+	if (access(dir_name, F_OK) == -1) {
+		char *temp_name;
+		char *parent_name;
+
+		temp_name = malloc(strlen(dir_name) + 1);
+		if (!temp_name) {
+			p_err("mem alloc failed");
+			return -1;
+		}
+
+		strcpy(temp_name, dir_name);
+		parent_name = dirname(temp_name);
+
+		if (is_bpffs(parent_name)) {
+			/* nothing to do if already mounted */
+			free(temp_name);
+			return err;
+		}
+
+		free(temp_name);
+
+		if (block_mount) {
+			p_err("no BPF file system found, not mounting it due to --nomount option");
+			return -1;
+		}
+
+		err = mkdir(dir_name, 0700);
+		if (err) {
+			p_err("failed to create dir (%s): %s", dir_name, strerror(errno));
+			return err;
+		}
+	} else if (block_mount) {
+		p_err("no BPF file system found, not mounting it due to --nomount option");
+		return -1;
+	}
+
+	err = mnt_fs(dir_name, "bpf", err_str, ERR_MAX_LEN);
+	if (err) {
+		err_str[ERR_MAX_LEN - 1] = '\0';
+		p_err("can't mount BPF file system on given dir (%s): %s",
+		      dir_name, err_str);
+	}
+
+	return err;
+}
+
+int mount_bpffs_given_file(const char *file_name)
+{
+	char err_str[ERR_MAX_LEN];
+	char *temp_name;
+	char *dir;
+	int err = 0;
+
+	if (access(file_name, F_OK) != -1) {
+		p_err("bpf object can't be pinned since file (%s) already exists", file_name);
+		return -1;
+	}
+
+	temp_name = malloc(strlen(file_name) + 1);
+	if (!temp_name) {
 		p_err("mem alloc failed");
 		return -1;
 	}
 
-	strcpy(file, name);
-	dir = dirname(file);
+	strcpy(temp_name, file_name);
+	dir = dirname(temp_name);
 
 	if (is_bpffs(dir))
 		/* nothing to do if already mounted */
@@ -277,11 +333,11 @@ int mount_bpffs_for_pin(const char *name, bool is_dir)
 	if (err) {
 		err_str[ERR_MAX_LEN - 1] = '\0';
 		p_err("can't mount BPF file system to pin the object (%s): %s",
-		      name, err_str);
+		      file_name, err_str);
 	}
 
 out_free:
-	free(file);
+	free(temp_name);
 	return err;
 }
 
@@ -289,7 +345,7 @@ int do_pin_fd(int fd, const char *name)
 {
 	int err;
 
-	err = mount_bpffs_for_pin(name, false);
+	err = mount_bpffs_given_file(name);
 	if (err)
 		return err;
 
diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c
index 6b0e5202ca7a..3911563dcc60 100644
--- a/tools/bpf/bpftool/iter.c
+++ b/tools/bpf/bpftool/iter.c
@@ -76,7 +76,7 @@ static int do_pin(int argc, char **argv)
 		goto close_obj;
 	}
 
-	err = mount_bpffs_for_pin(path, false);
+	err = mount_bpffs_given_file(path);
 	if (err)
 		goto close_link;
 
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index b8bb08d10dec..20e06ad183ec 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -142,7 +142,8 @@ const char *get_fd_type_name(enum bpf_obj_type type);
 char *get_fdinfo(int fd, const char *key);
 int open_obj_pinned(const char *path, bool quiet);
 int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type);
-int mount_bpffs_for_pin(const char *name, bool is_dir);
+int mount_bpffs_given_file(const char *file_name);
+int mount_bpffs_on_dir(const char *dir_name);
 int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(int *, char ***));
 int do_pin_fd(int fd, const char *name);
 
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 9cb42a3366c0..09b5f0415a5e 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1778,7 +1778,10 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 		goto err_close_obj;
 	}
 
-	err = mount_bpffs_for_pin(pinfile, !first_prog_only);
+	if (first_prog_only)
+		err = mount_bpffs_given_file(pinfile);
+	else
+		err = mount_bpffs_on_dir(pinfile);
 	if (err)
 		goto err_close_obj;
 
diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
index d573f2640d8e..bf50a99b2501 100644
--- a/tools/bpf/bpftool/struct_ops.c
+++ b/tools/bpf/bpftool/struct_ops.c
@@ -515,7 +515,7 @@ static int do_register(int argc, char **argv)
 	if (argc == 1)
 		linkdir = GET_ARG();
 
-	if (linkdir && mount_bpffs_for_pin(linkdir, true)) {
+	if (linkdir && mount_bpffs_on_dir(linkdir)) {
 		p_err("can't mount bpffs for pinning");
 		return -1;
 	}
-- 
2.44.0

Re: [PATCH bpf-next v2] bpftool: Mount bpffs on provided dir instead of parent dir

[Quentin Monnet]

Thanks! Apologies for the delay.

2024-03-08 13:07 UTC+0000 ~ Sahil Siddiq <icegambit91@gmail.com>
> When pinning programs/objects under PATH (eg: during "bpftool prog
> loadall") the bpffs is mounted on the parent dir of PATH in the
> following situations:
> - the given dir exists but it is not bpffs.
> - the given dir doesn't exist and the parent dir is not bpffs.
> 
> Mounting on the parent dir can also have the unintentional side-
> effect of hiding other files located under the parent dir.
> 
> If the given dir exists but is not bpffs, then the bpffs should
> be mounted on the given dir and not its parent dir.
> 
> Similarly, if the given dir doesn't exist and its parent dir is not
> bpffs, then the given dir should be created and the bpffs should be
> mounted on this new dir.
> 
> Link: https://lore.kernel.org/bpf/2da44d24-74ae-a564-1764-afccf395eeec@isovalent.com/T/#t
> 
> Closes: https://github.com/libbpf/bpftool/issues/100
> 
> Fixes: 2a36c26fe3b8 ("bpftool: Support bpffs mountpoint as pin path for prog loadall")
> 
> Signed-off-by: Sahil Siddiq <icegambit91@gmail.com>

Note: you don't need the blank lines between the tags.

> ---
> Changes since v1:
>  - Split "mount_bpffs_for_pin" into two functions:
>    - mount_bpffs_on_dir
>    - mount_bpffs_given_file
>    This is done to improve maintainability and readability.
>  - Improve error messages
>  - (mount_bpffs_given_file): If the file already exists, throw an
>    error instead of waiting for bpf_obj_pin() to throw an error
>  - Code style changes

You can keep the changelog as part of the patch description.

> 
>  tools/bpf/bpftool/common.c     | 78 +++++++++++++++++++++++++++++-----
>  tools/bpf/bpftool/iter.c       |  2 +-
>  tools/bpf/bpftool/main.h       |  3 +-
>  tools/bpf/bpftool/prog.c       |  5 ++-
>  tools/bpf/bpftool/struct_ops.c |  2 +-
>  5 files changed, 75 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index cc6e6aae2447..d2746681200e 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -244,24 +244,80 @@ int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type)
>  	return fd;
>  }
>  
> -int mount_bpffs_for_pin(const char *name, bool is_dir)
> +int mount_bpffs_on_dir(const char *dir_name)

With all the checks and the potential directory creation, we could maybe
rename this into "prepare_bpffs_dir()" or something like this?

>  {
>  	char err_str[ERR_MAX_LEN];
> -	char *file;
> -	char *dir;
>  	int err = 0;
>  
> -	if (is_dir && is_bpffs(name))
> +	if (is_bpffs(dir_name))
>  		return err;
>  
> -	file = malloc(strlen(name) + 1);
> -	if (!file) {
> +	if (access(dir_name, F_OK) == -1) {
> +		char *temp_name;
> +		char *parent_name;
> +
> +		temp_name = malloc(strlen(dir_name) + 1);
> +		if (!temp_name) {
> +			p_err("mem alloc failed");
> +			return -1;
> +		}
> +
> +		strcpy(temp_name, dir_name);
> +		parent_name = dirname(temp_name);
> +
> +		if (is_bpffs(parent_name)) {
> +			/* nothing to do if already mounted */
> +			free(temp_name);
> +			return err;
> +		}
> +
> +		free(temp_name);
> +
> +		if (block_mount) {
> +			p_err("no BPF file system found, not mounting it due to --nomount option");
> +			return -1;
> +		}
> +
> +		err = mkdir(dir_name, 0700);
> +		if (err) {
> +			p_err("failed to create dir (%s): %s", dir_name, strerror(errno));
> +			return err;
> +		}
> +	} else if (block_mount) {
> +		p_err("no BPF file system found, not mounting it due to --nomount option");
> +		return -1;
> +	}

I'd maybe change this block a little (although it's up to you):

	bool dir_exists;

	dire_exists = (access(...) == 0);
	if (!dir_exists) {
		...
		free(temp_name);
	}

	if (block_mount) {
		...
	}

	if (!dir_exists) {
		err = mkdir(...);
		...
	}

	err = mnt_fs(...);
	...

This would also enable us to remove the directory we just created, if
we're not able to mount the bpffs on it, before leaving the function.

> +
> +	err = mnt_fs(dir_name, "bpf", err_str, ERR_MAX_LEN);
> +	if (err) {
> +		err_str[ERR_MAX_LEN - 1] = '\0';
> +		p_err("can't mount BPF file system on given dir (%s): %s",
> +		      dir_name, err_str);
> +	}
> +
> +	return err;
> +}
> +
> +int mount_bpffs_given_file(const char *file_name)

I'd remove/replace "given" from the name, maybe "mount_bpffs_for_file"?

> +{
> +	char err_str[ERR_MAX_LEN];
> +	char *temp_name;
> +	char *dir;
> +	int err = 0;
> +
> +	if (access(file_name, F_OK) != -1) {
> +		p_err("bpf object can't be pinned since file (%s) already exists", file_name);

I'd remove "file", the existing object might be a directory and it might
be confusing.

> +		return -1;
> +	}
> +
> +	temp_name = malloc(strlen(file_name) + 1);
> +	if (!temp_name) {
>  		p_err("mem alloc failed");
>  		return -1;
>  	}
>  
> -	strcpy(file, name);
> -	dir = dirname(file);
> +	strcpy(temp_name, file_name);
> +	dir = dirname(temp_name);
>  

Here, we could check for the existence of "dir", and error out
otherwise. The reason is that with the current code, if dir does not
exist but user passes --nomount, then we fail to pin the path (given
that the directory is not present), but the message returned will be "no
BPF file system found, not mounting it due to --nomount option", which
is confusing.

Same note applies to the other function as well.

>  	if (is_bpffs(dir))
>  		/* nothing to do if already mounted */
> @@ -277,11 +333,11 @@ int mount_bpffs_for_pin(const char *name, bool is_dir)
>  	if (err) {
>  		err_str[ERR_MAX_LEN - 1] = '\0';
>  		p_err("can't mount BPF file system to pin the object (%s): %s",

We could also indicate the path where we tried to mount the bpffs, in
this message.

> -		      name, err_str);
> +		      file_name, err_str);
>  	}
>  
>  out_free:
> -	free(file);
> +	free(temp_name);
>  	return err;
>  }
>  
> @@ -289,7 +345,7 @@ int do_pin_fd(int fd, const char *name)
>  {
>  	int err;
>  
> -	err = mount_bpffs_for_pin(name, false);
> +	err = mount_bpffs_given_file(name);
>  	if (err)
>  		return err;
>  
> diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c
> index 6b0e5202ca7a..3911563dcc60 100644
> --- a/tools/bpf/bpftool/iter.c
> +++ b/tools/bpf/bpftool/iter.c
> @@ -76,7 +76,7 @@ static int do_pin(int argc, char **argv)
>  		goto close_obj;
>  	}
>  
> -	err = mount_bpffs_for_pin(path, false);
> +	err = mount_bpffs_given_file(path);
>  	if (err)
>  		goto close_link;
>  
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index b8bb08d10dec..20e06ad183ec 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -142,7 +142,8 @@ const char *get_fd_type_name(enum bpf_obj_type type);
>  char *get_fdinfo(int fd, const char *key);
>  int open_obj_pinned(const char *path, bool quiet);
>  int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type);
> -int mount_bpffs_for_pin(const char *name, bool is_dir);
> +int mount_bpffs_given_file(const char *file_name);
> +int mount_bpffs_on_dir(const char *dir_name);
>  int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(int *, char ***));
>  int do_pin_fd(int fd, const char *name);
>  
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 9cb42a3366c0..09b5f0415a5e 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1778,7 +1778,10 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>  		goto err_close_obj;
>  	}
>  
> -	err = mount_bpffs_for_pin(pinfile, !first_prog_only);
> +	if (first_prog_only)
> +		err = mount_bpffs_given_file(pinfile);
> +	else
> +		err = mount_bpffs_on_dir(pinfile);
>  	if (err)
>  		goto err_close_obj;
>  
> diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
> index d573f2640d8e..bf50a99b2501 100644
> --- a/tools/bpf/bpftool/struct_ops.c
> +++ b/tools/bpf/bpftool/struct_ops.c
> @@ -515,7 +515,7 @@ static int do_register(int argc, char **argv)
>  	if (argc == 1)
>  		linkdir = GET_ARG();
>  
> -	if (linkdir && mount_bpffs_for_pin(linkdir, true)) {
> +	if (linkdir && mount_bpffs_on_dir(linkdir)) {
>  		p_err("can't mount bpffs for pinning");
>  		return -1;
>  	}

Other than the comments above, the patch works well, and the different
cases are much easier to follow than in v1, thanks!

I've checked that the programs load as expected, the directories are
created (or not) as expected, and the bpffs is mounted (or not) as
expected, for all the cases I could think of, with the following
commands (copied here, for the record), all worked as I expected:

    mkdir -p /sys/fs/bpf/some_dir
    mkdir -p /tmp/test-dir/some_dir

    # /tmp/ret.o contains 1 program, /tmp/rets.o contains several ones

    # Load one program under bpffs
    bpftool prog load /tmp/ret.o /sys/fs/bpf/foo type xdp
    mount | grep bpf
    ls /sys/fs/bpf/foo

    # Try to load one program when pin path exists, under bpffs
    bpftool prog load /tmp/ret.o /sys/fs/bpf/foo type xdp
    mount | grep bpf
    ls /sys/fs/bpf/foo
    rm /sys/fs/bpf/foo

    # Load one program under bpffs, with --nomount
    bpftool --nomount prog load /tmp/ret.o /sys/fs/bpf/foo type xdp
    mount | grep bpf
    ls /sys/fs/bpf/foo
    rm /sys/fs/bpf/foo

    # Load one program outside of bpffs, with --nomount
    bpftool --nomount prog load /tmp/ret.o /tmp/test-dir/foo type xdp
    mount | grep bpf
    ls /tmp/test-dir/foo
    # Load one program outside of bpffs
    bpftool prog load /tmp/ret.o /tmp/test-dir/foo type xdp
    mount | grep bpf
    ls /tmp/test-dir/foo
    # Try to load one program when pin path exists, outside of bpffs
    bpftool prog load /tmp/ret.o /tmp/test-dir/foo type xdp
    mount | grep bpf
    rm /tmp/test-dir/foo
    umount /tmp/test-dir
    umount /tmp/test-dir

    # Load multiple programs under bpffs, directory exists
    bpftool prog loadall /tmp/rets.o /sys/fs/bpf/some_dir
    mount | grep bpf
    ls /sys/fs/bpf/some_dir
    rm /sys/fs/bpf/some_dir/*
    # Load multiple programs under bpffs, with -n, directory exists
    bpftool --nomount prog loadall /tmp/rets.o /sys/fs/bpf/some_dir
    mount | grep bpf
    ls /sys/fs/bpf/some_dir
    rm /sys/fs/bpf/some_dir/*

    # Load multiple programs under bpffs, directory does not exist
    bpftool prog loadall /tmp/rets.o /sys/fs/bpf/new_dir
    mount | grep bpf
    ls /sys/fs/bpf/new_dir
    rm -r /sys/fs/bpf/new_dir
    # Load multiple progs under bpffs, with -n, directory doesn't exist
    bpftool --nomount prog loadall /tmp/rets.o /sys/fs/bpf/new_dir
    mount | grep bpf
    ls /sys/fs/bpf/new_dir
    rm -r /sys/fs/bpf/new_dir

    # Load multiple programs outside of bpffs, directory exists
    bpftool prog loadall /tmp/rets.o /tmp/test-dir/some_dir
    mount | grep bpf
    ls /tmp/test-dir/some_dir
    rm /tmp/test-dir/some_dir/*
    umount /tmp/test-dir/some_dir
    umount /tmp/test-dir/some_dir
    # Try to load multiple progs outside of bpffs, with -n, dir exists
    bpftool --nomount prog loadall /tmp/rets.o /tmp/test-dir/some_dir
    mount | grep bpf
    ls /tmp/test-dir/some_dir

    # Load multiple programs outside of bpffs, directory does not exist
    bpftool prog loadall /tmp/rets.o /tmp/test-dir/new_dir
    mount | grep bpf
    ls /tmp/test-dir/new_dir
    umount /tmp/test-dir/new_dir
    umount /tmp/test-dir/new_dir
    rm -r /tmp/test-dir/new_dir
    # Try to load multiple progs outside of bpffs, with -n, new dir
    bpftool --nomount prog loadall /tmp/rets.o /tmp/test-dir/new_dir
    mount | grep bpf
    ls /tmp/test-dir/new_dir
    rm -r /tmp/test-dir

Thanks,
Quentin

Re: [PATCH bpf-next v2] bpftool: Mount bpffs on provided dir instead of parent dir

[Sahil]

Hi,

On Wednesday, March 13, 2024 9:17:44 PM IST Quentin Monnet wrote:
> Thanks! Apologies for the delay.

No worries! Thank you for the review.

> [...]
> Note: you don't need the blank lines between the tags.
> [...]
> You can keep the changelog as part of the patch description.

Got it. I'll keep this in mind when I submit v3.

> [...]
> With all the checks and the potential directory creation, we could maybe
> rename this into "prepare_bpffs_dir()" or something like this?

"prepare_bpffs_dir" is quite apt. If longer names are acceptable then I
would also recommend "prepare_and_mount_bpffs_dir" so it indicates
that it'll also mount the bpffs on the dir (when relevant) after performing
the checks.

> [...]
> I'd maybe change this block a little (although it's up to you):
> 
> 	bool dir_exists;
> 
> 	dire_exists = (access(...) == 0);
> 	if (!dir_exists) {
> 		...
> 		free(temp_name);
> 	}
> 
> 	if (block_mount) {
> 		...
> 	}
> 
> 	if (!dir_exists) {
> 		err = mkdir(...);
> 		...
> 	}
> 
> 	err = mnt_fs(...);
> 	...
> 
> This would also enable us to remove the directory we just created, if
> we're not able to mount the bpffs on it, before leaving the function.

I agree with this. This will also keep the implementation a little more
succinct with just one "block_mount" conditional block.

> [...]
> I'd remove/replace "given" from the name, maybe "mount_bpffs_for_file"?

This is definitely a better name. I am not very good when it comes to
making up names :P

> [...]
> I'd remove "file", the existing object might be a directory and it might
> be confusing.
> 
> > +		return -1;
> > +	}
> > +
> > +	temp_name = malloc(strlen(file_name) + 1);
> > +	if (!temp_name) {
> > 
> >  		p_err("mem alloc failed");
> >  		return -1;
> >  	
> >  	}
> > 
> > -	strcpy(file, name);
> > -	dir = dirname(file);
> > +	strcpy(temp_name, file_name);
> > +	dir = dirname(temp_name);
> 
> Here, we could check for the existence of "dir", and error out
> otherwise. The reason is that with the current code, if dir does not
> exist but user passes --nomount, then we fail to pin the path (given
> that the directory is not present), but the message returned will be "no
> BPF file system found, not mounting it due to --nomount option", which
> is confusing.
> 
> Same note applies to the other function as well.
> 
> >  	if (is_bpffs(dir))
> >  	
> >  		/* nothing to do if already mounted */
> > 
> > @@ -277,11 +333,11 @@ int mount_bpffs_for_pin(const char *name, bool
> > is_dir)> 
> >  	if (err) {
> >  	
> >  		err_str[ERR_MAX_LEN - 1] = '\0';
> >  		p_err("can't mount BPF file system to pin the object (%s): %s",
> 
> We could also indicate the path where we tried to mount the bpffs, in
> this message.

Understood. I'll make these changes as well.

> [...]
> Other than the comments above, the patch works well, and the different
> cases are much easier to follow than in v1, thanks!
> 
> I've checked that the programs load as expected, the directories are
> created (or not) as expected, and the bpffs is mounted (or not) as
> expected, for all the cases I could think of, with the following
> commands (copied here, for the record), all worked as I expected:

That's really nice to hear. I'll incorporate the recommended changes and
will send v3 soon.

Thanks,
Sahil

Re: [PATCH bpf-next v2] bpftool: Mount bpffs on provided dir instead of parent dir

[Quentin Monnet]

2024-03-14 21:16 UTC+0000 ~ Sahil <icegambit91@gmail.com>
> Hi,
> 
> On Wednesday, March 13, 2024 9:17:44 PM IST Quentin Monnet wrote:
>> Thanks! Apologies for the delay.
> 
> No worries! Thank you for the review.
> 
>> [...]
>> Note: you don't need the blank lines between the tags.
>> [...]
>> You can keep the changelog as part of the patch description.
> 
> Got it. I'll keep this in mind when I submit v3.
> 
>> [...]
>> With all the checks and the potential directory creation, we could maybe
>> rename this into "prepare_bpffs_dir()" or something like this?
> 
> "prepare_bpffs_dir" is quite apt. If longer names are acceptable then I
> would also recommend "prepare_and_mount_bpffs_dir" so it indicates
> that it'll also mount the bpffs on the dir (when relevant) after performing
> the checks.

The length looks acceptable to me. I used "prepare" to summarise "create
and mount", if you prefer "mount" explicitly then maybe
"create_and_mount_bpffs_dir()"? There's no other preparation step as far
as I remember so we should as well make it clear.

> That's really nice to hear. I'll incorporate the recommended changes and
> will send v3 soon.

Thanks a lot,
Quentin

Re: [PATCH bpf-next v2] bpftool: Mount bpffs on provided dir instead of parent dir

[Sahil]

Hi,

Sorry for the delay in my response. I have made
the relevant changes and have sent v3.

Thanks,
Sahil