From d3e2a7f7e6f1167b38ee333a34d610e165252a36 Mon Sep 17 00:00:00 2001 From: Alexander Wilson Date: Fri, 22 Jul 2022 04:15:08 -0700 Subject: [PATCH 1/3] copy.[ch]: Refactor - Refactor: Move HardlinkContext to header file - Refactor: Create `fd_copy_tree_generic` which isolates the functionality to check stat type and appropriately copy. - Refactor: Create `fd_copy_leaf` which handles copying leaf nodes of a file tree. --- src/shared/copy.c | 98 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 75 insertions(+), 23 deletions(-) diff --git a/src/shared/copy.c b/src/shared/copy.c index 4d52b4c26b..3c5f296d2d 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -676,6 +676,23 @@ static int memorize_hardlink( return 1; } +static int fd_copy_tree_generic( + int df, + const char *from, + const struct stat *st, + int dt, + const char *to, + dev_t original_device, + unsigned depth_left, + uid_t override_uid, + gid_t override_gid, + CopyFlags copy_flags, + HardlinkContext *hardlink_context, + const char *display_path, + copy_progress_path_t progress_path, + copy_progress_bytes_t progress_bytes, + void *userdata); + static int fd_copy_regular( int df, const char *from, @@ -992,18 +1009,9 @@ static int fd_copy_directory( if (r > 0) continue; } + } - q = fd_copy_directory(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, depth_left-1, override_uid, override_gid, copy_flags, hardlink_context, child_display_path, progress_path, progress_bytes, userdata); - } else if (S_ISREG(buf.st_mode)) - q = fd_copy_regular(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags, hardlink_context, progress_bytes, userdata); - else if (S_ISLNK(buf.st_mode)) - q = fd_copy_symlink(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags); - else if (S_ISFIFO(buf.st_mode)) - q = fd_copy_fifo(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags, hardlink_context); - else if (S_ISBLK(buf.st_mode) || S_ISCHR(buf.st_mode) || S_ISSOCK(buf.st_mode)) - q = fd_copy_node(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags, hardlink_context); - else - q = -EOPNOTSUPP; + q = fd_copy_tree_generic(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, depth_left-1, override_uid, override_gid, copy_flags, hardlink_context, child_display_path, progress_path, progress_bytes, userdata); if (q == -EINTR) /* Propagate SIGINT/SIGTERM up instantly */ return q; @@ -1034,6 +1042,61 @@ static int fd_copy_directory( return r; } +static int fd_copy_leaf( + int df, + const char *from, + const struct stat *st, + int dt, + const char *to, + uid_t override_uid, + gid_t override_gid, + CopyFlags copy_flags, + HardlinkContext *hardlink_context, + const char *display_path, + copy_progress_bytes_t progress_bytes, + void *userdata) { + int r; + + if (S_ISREG(st->st_mode)) + r = fd_copy_regular(df, from, st, dt, to, override_uid, override_gid, copy_flags, hardlink_context, progress_bytes, userdata); + else if (S_ISLNK(st->st_mode)) + r = fd_copy_symlink(df, from, st, dt, to, override_uid, override_gid, copy_flags); + else if (S_ISFIFO(st->st_mode)) + r = fd_copy_fifo(df, from, st, dt, to, override_uid, override_gid, copy_flags, hardlink_context); + else if (S_ISBLK(st->st_mode) || S_ISCHR(st->st_mode) || S_ISSOCK(st->st_mode)) + r = fd_copy_node(df, from, st, dt, to, override_uid, override_gid, copy_flags, hardlink_context); + else + r = -EOPNOTSUPP; + + return r; +} + +static int fd_copy_tree_generic( + int df, + const char *from, + const struct stat *st, + int dt, + const char *to, + dev_t original_device, + unsigned depth_left, + uid_t override_uid, + gid_t override_gid, + CopyFlags copy_flags, + HardlinkContext *hardlink_context, + const char *display_path, + copy_progress_path_t progress_path, + copy_progress_bytes_t progress_bytes, + void *userdata) { + int r; + + if (S_ISDIR(st->st_mode)) + r = fd_copy_directory(df, from, st, dt, to, original_device, depth_left-1, override_uid, override_gid, copy_flags, hardlink_context, display_path, progress_path, progress_bytes, userdata); + else + r = fd_copy_leaf(df, from, st, dt, to, override_uid, override_gid, copy_flags, hardlink_context, display_path, progress_bytes, userdata); + + return r; +} + int copy_tree_at_full( int fdf, const char *from, @@ -1055,18 +1118,7 @@ int copy_tree_at_full( if (fstatat(fdf, from, &st, AT_SYMLINK_NOFOLLOW) < 0) return -errno; - if (S_ISREG(st.st_mode)) - r = fd_copy_regular(fdf, from, &st, fdt, to, override_uid, override_gid, copy_flags, NULL, progress_bytes, userdata); - else if (S_ISDIR(st.st_mode)) - r = fd_copy_directory(fdf, from, &st, fdt, to, st.st_dev, COPY_DEPTH_MAX, override_uid, override_gid, copy_flags, NULL, NULL, progress_path, progress_bytes, userdata); - else if (S_ISLNK(st.st_mode)) - r = fd_copy_symlink(fdf, from, &st, fdt, to, override_uid, override_gid, copy_flags); - else if (S_ISFIFO(st.st_mode)) - r = fd_copy_fifo(fdf, from, &st, fdt, to, override_uid, override_gid, copy_flags, NULL); - else if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode) || S_ISSOCK(st.st_mode)) - r = fd_copy_node(fdf, from, &st, fdt, to, override_uid, override_gid, copy_flags, NULL); - else - return -EOPNOTSUPP; + r = fd_copy_tree_generic(fdf, from, &st, fdt, to, st.st_dev, COPY_DEPTH_MAX, override_uid, override_gid, copy_flags, NULL, NULL, progress_path, progress_bytes, userdata); if (r < 0) return r; From 922409558e46f855a8fba2c666c056fd95dc2428 Mon Sep 17 00:00:00 2001 From: Alexander Wilson Date: Fri, 22 Jul 2022 04:13:31 -0700 Subject: [PATCH 2/3] copy: Respect COPY_REPLACE flag for copy_tree - Add a test that asserts that copy_tree on an existing file will fail without COPY_REPLACE - Add a test that asserts that copy_tree with COPY_MERGE and COPY_REPLACE on an existing directory will overwrite files that already exist. --- src/shared/copy.c | 12 ++++++-- src/test/test-copy.c | 69 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/src/shared/copy.c b/src/shared/copy.c index 3c5f296d2d..182544a206 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -1090,9 +1090,17 @@ static int fd_copy_tree_generic( int r; if (S_ISDIR(st->st_mode)) - r = fd_copy_directory(df, from, st, dt, to, original_device, depth_left-1, override_uid, override_gid, copy_flags, hardlink_context, display_path, progress_path, progress_bytes, userdata); - else + return fd_copy_directory(df, from, st, dt, to, original_device, depth_left-1, override_uid, override_gid, copy_flags, hardlink_context, display_path, progress_path, progress_bytes, userdata); + + r = fd_copy_leaf(df, from, st, dt, to, override_uid, override_gid, copy_flags, hardlink_context, display_path, progress_bytes, userdata); + /* We just tried to copy a leaf node of the tree. If it failed because the node already exists *and* the COPY_REPLACE flag has been provided, we should unlink the node and re-copy. */ + if (r == -EEXIST && (copy_flags & COPY_REPLACE)) { + /* This codepath is us trying to address an error to copy, if the unlink fails, lets just return the original error. */ + if (unlinkat(dt, to, 0) < 0) + return r; + r = fd_copy_leaf(df, from, st, dt, to, override_uid, override_gid, copy_flags, hardlink_context, display_path, progress_bytes, userdata); + } return r; } diff --git a/src/test/test-copy.c b/src/test/test-copy.c index a669df644b..09e78b920e 100644 --- a/src/test/test-copy.c +++ b/src/test/test-copy.c @@ -50,6 +50,75 @@ TEST(copy_file) { unlink(fn_copy); } +static bool read_file_and_streq(const char* filepath, const char* expected_contents) { + _cleanup_free_ char *buf = NULL; + + assert_se(read_full_file(filepath, &buf, NULL) == 0); + return streq(buf, expected_contents); +} + +TEST(copy_tree_replace_file) { + _cleanup_free_ char *src = NULL, *dst = NULL; + + assert_se(tempfn_random("/tmp/test-copy_file.XXXXXX", NULL, &src) >= 0); + assert_se(tempfn_random("/tmp/test-copy_file.XXXXXX", NULL, &dst) >= 0); + + assert_se(write_string_file(src, "bar bar", WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(dst, "foo foo foo", WRITE_STRING_FILE_CREATE) == 0); + + /* The file exists- now overwite original contents, and test the COPY_REPLACE flag. */ + + assert_se(copy_tree(src, dst, UID_INVALID, GID_INVALID, COPY_REFLINK) == -EEXIST); + + assert_se(read_file_and_streq(dst, "foo foo foo\n")); + + assert_se(copy_tree(src, dst, UID_INVALID, GID_INVALID, COPY_REFLINK|COPY_REPLACE) == 0); + + assert_se(read_file_and_streq(dst, "bar bar\n")); +} + +TEST(copy_tree_replace_dirs) { + _cleanup_free_ char *src_path1 = NULL, *src_path2 = NULL, *dst_path1 = NULL, *dst_path2 = NULL; + _cleanup_(rm_rf_physical_and_freep) char *src_directory = NULL, *dst_directory = NULL; + const char *file1 = "foo_file", *file2 = "bar_file"; + + /* Create the random source/destination directories */ + assert_se(mkdtemp_malloc("/tmp/dirXXXXXX", &src_directory) >= 0); + assert_se(mkdtemp_malloc("/tmp/dirXXXXXX", &dst_directory) >= 0); + + /* Construct the source/destination filepaths (should have different dir name, but same file names within) */ + assert_se(src_path1 = path_join(src_directory, file1)); + assert_se(src_path2 = path_join(src_directory, file2)); + assert_se(dst_path1 = path_join(dst_directory, file1)); + assert_se(dst_path2 = path_join(dst_directory, file2)); + + /* Populate some data to differentiate the files. */ + assert_se(write_string_file(src_path1, "src file 1", WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(src_path2, "src file 2", WRITE_STRING_FILE_CREATE) == 0); + + assert_se(write_string_file(dst_path1, "dest file 1", WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(dst_path2, "dest file 2", WRITE_STRING_FILE_CREATE) == 0); + + /* Copying without COPY_REPLACE should fail because the destination file already exists. */ + assert_se(copy_tree(src_directory, dst_directory, UID_INVALID, GID_INVALID, COPY_REFLINK) == -EEXIST); + + { + assert_se(read_file_and_streq(src_path1, "src file 1\n")); + assert_se(read_file_and_streq(src_path2, "src file 2\n")); + assert_se(read_file_and_streq(dst_path1, "dest file 1\n")); + assert_se(read_file_and_streq(dst_path2, "dest file 2\n")); + } + + assert_se(copy_tree(src_directory, dst_directory, UID_INVALID, GID_INVALID, COPY_REFLINK|COPY_REPLACE|COPY_MERGE) == 0); + + { + assert_se(read_file_and_streq(src_path1, "src file 1\n")); + assert_se(read_file_and_streq(src_path2, "src file 2\n")); + assert_se(read_file_and_streq(dst_path1, "src file 1\n")); + assert_se(read_file_and_streq(dst_path2, "src file 2\n")); + } +} + TEST(copy_file_fd) { char in_fn[] = "/tmp/test-copy-file-fd-XXXXXX"; char out_fn[] = "/tmp/test-copy-file-fd-XXXXXX"; From ae03e1a97229b192e50439091db92a1b11a8df93 Mon Sep 17 00:00:00 2001 From: Alexander Wilson Date: Fri, 22 Jul 2022 04:08:11 -0700 Subject: [PATCH 3/3] machinectl: Add plumbing for a `--force` flag for file copy machine: Add APIs CopyTo[Machine]WithFlags + CopyFrom[Machine]WithFlags - Same API to those without `WithFlags` (except this can take flags) - Initially, only a flag to allow replacing a file if it already exists --- man/machinectl.xml | 4 ++-- man/org.freedesktop.machine1.xml | 34 ++++++++++++++++++++++++++++++-- src/machine/machine-dbus.c | 24 ++++++++++++++++++++++ src/machine/machine-dbus.h | 5 +++++ src/machine/machinectl.c | 16 ++++++++++++++- src/machine/machined-dbus.c | 10 ++++++++++ 6 files changed, 88 insertions(+), 5 deletions(-) diff --git a/man/machinectl.xml b/man/machinectl.xml index 7ecb8885a2..5bc82e5d1a 100644 --- a/man/machinectl.xml +++ b/man/machinectl.xml @@ -305,7 +305,7 @@ - copy-to NAME PATH [PATH] + copy-to NAME PATH [PATH] Copies files or directories from the host system into a running container. Takes a container name, @@ -319,7 +319,7 @@ - copy-from NAME PATH [PATH] + copy-from NAME PATH [PATH] Copies files or directories from a container into the host system. Takes a container name, followed by the diff --git a/man/org.freedesktop.machine1.xml b/man/org.freedesktop.machine1.xml index 946041acfc..c366c1b19a 100644 --- a/man/org.freedesktop.machine1.xml +++ b/man/org.freedesktop.machine1.xml @@ -116,6 +116,14 @@ node /org/freedesktop/machine1 { CopyToMachine(in s name, in s source, in s destination); + CopyFromMachineWithFlags(in s name, + in s source, + in s destination, + in t flags); + CopyToMachineWithFlags(in s name, + in s source, + in s destination, + in t flags); OpenMachineRootDirectory(in s name, out h fd); GetMachineUIDShift(in s name, @@ -176,6 +184,8 @@ node /org/freedesktop/machine1 { + + @@ -236,6 +246,10 @@ node /org/freedesktop/machine1 { + + + + @@ -394,8 +408,10 @@ node /org/freedesktop/machine1 { CopyFromMachine() copies files or directories from a container into the host. It takes a container name, a source directory in the container and a destination directory on the - host as arguments. CopyToMachine() does the opposite and copies files from a source - directory on the host into a destination directory in the container. + host as arguments. + CopyToMachine() does the opposite and copies files from a source + directory on the host into a destination directory in the container. + CopyFromMachineWithFlags() and CopyToMachineWithFlags do the same but take an additional flags argument. RemoveImage() removes the image with the specified name. @@ -465,6 +481,12 @@ node /org/freedesktop/machine1/machine/rawhide { in s destination); CopyTo(in s source, in s destination); + CopyFromWithFlags(in s source, + in s destination, + in t flags); + CopyToWithFlags(in s source, + in s destination, + in t flags); OpenRootDirectory(out h fd); properties: @org.freedesktop.DBus.Property.EmitsChangedSignal("const") @@ -510,6 +532,10 @@ node /org/freedesktop/machine1/machine/rawhide { + + + + @@ -540,6 +566,10 @@ node /org/freedesktop/machine1/machine/rawhide { + + + + diff --git a/src/machine/machine-dbus.c b/src/machine/machine-dbus.c index 293be0bf8b..7a07d20e0b 100644 --- a/src/machine/machine-dbus.c +++ b/src/machine/machine-dbus.c @@ -928,6 +928,20 @@ int bus_machine_method_copy(sd_bus_message *message, void *userdata, sd_bus_erro if (r < 0) return r; + if (endswith(sd_bus_message_get_member(message), "WithFlags")) { + uint64_t raw_flags; + + r = sd_bus_message_read(message, "t", &raw_flags); + if (r < 0) + return r; + + if ((raw_flags & ~_MACHINE_COPY_FLAGS_MASK_PUBLIC) != 0) + return -EINVAL; + + if (raw_flags & MACHINE_COPY_REPLACE) + copy_flags |= COPY_REPLACE; + } + if (!path_is_absolute(src)) return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Source path must be absolute."); @@ -1328,6 +1342,16 @@ static const sd_bus_vtable machine_vtable[] = { SD_BUS_NO_RESULT, bus_machine_method_copy, SD_BUS_VTABLE_UNPRIVILEGED), + SD_BUS_METHOD_WITH_ARGS("CopyFromWithFlags", + SD_BUS_ARGS("s", source, "s", destination, "t", flags), + SD_BUS_NO_RESULT, + bus_machine_method_copy, + SD_BUS_VTABLE_UNPRIVILEGED), + SD_BUS_METHOD_WITH_ARGS("CopyToWithFlags", + SD_BUS_ARGS("s", source, "s", destination, "t", flags), + SD_BUS_NO_RESULT, + bus_machine_method_copy, + SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_METHOD_WITH_ARGS("OpenRootDirectory", SD_BUS_NO_ARGS, SD_BUS_RESULT("h", fd), diff --git a/src/machine/machine-dbus.h b/src/machine/machine-dbus.h index 1c114f47c3..a0133451ab 100644 --- a/src/machine/machine-dbus.h +++ b/src/machine/machine-dbus.h @@ -6,6 +6,11 @@ #include "bus-util.h" #include "machine.h" +typedef enum { + MACHINE_COPY_REPLACE = 1 << 0, /* Public API via DBUS, do not change */ + _MACHINE_COPY_FLAGS_MASK_PUBLIC = MACHINE_COPY_REPLACE, +} MachineCopyFlags; + extern const BusObjectImplementation machine_object; char *machine_bus_path(Machine *s); diff --git a/src/machine/machinectl.c b/src/machine/machinectl.c index 213879c8ed..ef600e8ee4 100644 --- a/src/machine/machinectl.c +++ b/src/machine/machinectl.c @@ -34,6 +34,7 @@ #include "locale-util.h" #include "log.h" #include "logs-show.h" +#include "machine-dbus.h" #include "macro.h" #include "main-func.h" #include "mkdir.h" @@ -1093,6 +1094,13 @@ static int terminate_machine(int argc, char *argv[], void *userdata) { return 0; } +static const char *select_copy_method(bool copy_from, bool force) { + if (force) + return copy_from ? "CopyFromMachineWithFlags" : "CopyToMachineWithFlags"; + else + return copy_from ? "CopyFromMachine" : "CopyToMachine"; +} + static int copy_files(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; @@ -1123,7 +1131,7 @@ static int copy_files(int argc, char *argv[], void *userdata) { bus, &m, bus_machine_mgr, - copy_from ? "CopyFromMachine" : "CopyToMachine"); + select_copy_method(copy_from, arg_force)); if (r < 0) return bus_log_create_error(r); @@ -1136,6 +1144,12 @@ static int copy_files(int argc, char *argv[], void *userdata) { if (r < 0) return bus_log_create_error(r); + if (arg_force) { + r = sd_bus_message_append(m, "t", MACHINE_COPY_REPLACE); + if (r < 0) + return bus_log_create_error(r); + } + /* This is a slow operation, hence turn off any method call timeouts */ r = sd_bus_call(bus, m, USEC_INFINITY, &error, NULL); if (r < 0) diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c index 865d8400e8..794e353caa 100644 --- a/src/machine/machined-dbus.c +++ b/src/machine/machined-dbus.c @@ -1110,6 +1110,16 @@ const sd_bus_vtable manager_vtable[] = { SD_BUS_NO_RESULT, method_copy_machine, SD_BUS_VTABLE_UNPRIVILEGED), + SD_BUS_METHOD_WITH_ARGS("CopyFromMachineWithFlags", + SD_BUS_ARGS("s", name, "s", source, "s", destination, "t", flags), + SD_BUS_NO_RESULT, + method_copy_machine, + SD_BUS_VTABLE_UNPRIVILEGED), + SD_BUS_METHOD_WITH_ARGS("CopyToMachineWithFlags", + SD_BUS_ARGS("s", name, "s", source, "s", destination, "t", flags), + SD_BUS_NO_RESULT, + method_copy_machine, + SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_METHOD_WITH_ARGS("OpenMachineRootDirectory", SD_BUS_ARGS("s", name), SD_BUS_RESULT("h", fd),