Hi Sahil, sorry for the delay, and thanks for the v3! I've got some
minor comments on the error messages, but the rest of the code looks
mostly good - although you forgot to rename a function and it currently
fails to compile. Please see inline below.
On 21/03/2024 19:19, Sahil Siddiq wrote:
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@xxxxxxxxxxxxx/T/#t
Closes: https://github.com/libbpf/bpftool/issues/100
Fixes: 2a36c26fe3b8 ("bpftool: Support bpffs mountpoint as pin path for prog loadall")
Changes since v1:
- Split "mount_bpffs_for_pin" into two functions.
This is done to improve maintainability and readability.
Changes since v2:
- mount_bpffs_for_pin: rename to "create_and_mount_bpffs_dir".
- mount_bpffs_given_file: rename to "mount_bpffs_given_file".
That's the same!
- create_and_mount_bpffs_dir:
- introduce "dir_exists" boolean.
- remove new dir if "mnt_fs" fails.
- improve error handling and error messages.
Signed-off-by: Sahil Siddiq <icegambit91@xxxxxxxxx>
---
tools/bpf/bpftool/common.c | 93 ++++++++++++++++++++++++++++++----
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, 90 insertions(+), 15 deletions(-)
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index cc6e6aae2447..dce73105b0a5 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -244,24 +244,95 @@ 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 create_and_mount_bpffs_dir(const char *dir_name)
{
char err_str[ERR_MAX_LEN];
- char *file;
- char *dir;
int err = 0;
+ bool dir_exists;
Nit: "reverse Christmas tree order": In the absence of other criteria,
we tend to sort the variable definitions by line length. Please swap
dir_exists and err.
- if (is_dir && is_bpffs(name))
+ if (is_bpffs(dir_name))
return err;
- file = malloc(strlen(name) + 1);
- if (!file) {
+ dir_exists = (access(dir_name, F_OK) == 0);
+
+ if (!dir_exists) {
+ 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;
+ }
+
+ if (access(parent_name, F_OK) == -1) {
+ p_err("bpf object can't be pinned since dir (%s) does not exist",
Maybe "can't create directory '%s' to pin BPF object: parent dir %s does
not exist"?
+ parent_name);
+ free(temp_name);
+ return -1;
+ }
+
+ free(temp_name);
+ }
+
+ if (block_mount) {
+ p_err("no BPF file system found, not mounting it due to --nomount option");
+ return -1;
+ }
+
+ if (!dir_exists) {
+ err = mkdir(dir_name, 0700);
+ if (err) {
+ p_err("failed to create dir (%s): %s", dir_name, strerror(errno));
Or "failed to create directory '%s': %s".
+ return err;
+ }
+ }
+
+ 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);
+
+ if (!dir_exists) {
+ if (rmdir(dir_name) == -1)
+ p_err("failed to remove newly created dir (%s): %s",
+ dir_name, strerror(errno));
We're already on an error path, I'd maybe drop the check on the return
value from rmdir(). It doesn't matter much if it fails, I think.
+ }
+ }
+
+ return err;
+}
+
+int mount_bpffs_given_file(const char *file_name)
This function must be renamed, as you did at the call sites.
+{
+ 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);
Not sure why the parenthesis, let's use quotes? Can we also replace
"since" with "because", or "given that", or simply a colon, please? And
I'd drop the passive voice, here's what I would use:
"can't pin BPF object: path '%s' already exists"
+ 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 +348,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",
"object '%s'" instead of "the object (%s)" please.
- name, err_str);
+ file_name, err_str);
}
out_free:
- free(file);
+ free(temp_name);
return err;
}
@@ -289,7 +360,7 @@ int do_pin_fd(int fd, const char *name)
{
int err;
- err = mount_bpffs_for_pin(name, false);
+ err = mount_bpffs_for_file(name);
Compiler says "undefined reference to `mount_bpffs_for_file`", please
build and test your patch. See also how most CI jobs fail:
https://github.com/kernel-patches/bpf/pull/6614
Quentin
