Merge pull request #3596 from poettering/machine-clean

make "machinectl clean" asynchronous, and open it up via PolicyKit

7 files changed, 233 insertions, 57 deletions

diff --git a/src/machine/image-dbus.c b/src/machine/image-dbus.c
index 0eed9b81bb..867bbc467b 100644
--- a/src/machine/image-dbus.c
+++ b/src/machine/image-dbus.c
@@ -81,7 +81,7 @@ int bus_image_method_remove(
 
         errno_pipe_fd[1] = safe_close(errno_pipe_fd[1]);
 
-        r = operation_new(m, NULL, child, message, errno_pipe_fd[0]);
+        r = operation_new(m, NULL, child, message, errno_pipe_fd[0], NULL);
         if (r < 0) {
                 (void) sigkill_wait(child);
                 return r;
@@ -193,7 +193,7 @@ int bus_image_method_clone(
 
         errno_pipe_fd[1] = safe_close(errno_pipe_fd[1]);
 
-        r = operation_new(m, NULL, child, message, errno_pipe_fd[0]);
+        r = operation_new(m, NULL, child, message, errno_pipe_fd[0], NULL);
         if (r < 0) {
                 (void) sigkill_wait(child);
                 return r;
diff --git a/src/machine/machine-dbus.c b/src/machine/machine-dbus.c
index de5d98f23e..ba7ac04b56 100644
--- a/src/machine/machine-dbus.c
+++ b/src/machine/machine-dbus.c
@@ -1200,8 +1200,6 @@ int bus_machine_method_copy(sd_bus_message *message, void *userdata, sd_bus_erro
 
         child_fail:
                 (void) write(errno_pipe_fd[1], &r, sizeof(r));
-                errno_pipe_fd[1] = safe_close(errno_pipe_fd[1]);
-
                 _exit(EXIT_FAILURE);
         }
 
@@ -1209,7 +1207,7 @@ int bus_machine_method_copy(sd_bus_message *message, void *userdata, sd_bus_erro
 
         /* Copying might take a while, hence install a watch on the child, and return */
 
-        r = operation_new(m->manager, m, child, message, errno_pipe_fd[0]);
+        r = operation_new(m->manager, m, child, message, errno_pipe_fd[0], NULL);
         if (r < 0) {
                 (void) sigkill_wait(child);
                 return r;
diff --git a/src/machine/machinectl.c b/src/machine/machinectl.c
index 843e678eca..161dd3922b 100644
--- a/src/machine/machinectl.c
+++ b/src/machine/machinectl.c
@@ -2418,7 +2418,7 @@ static int set_limit(int argc, char *argv[], void *userdata) {
 }
 
 static int clean_images(int argc, char *argv[], void *userdata) {
-        _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
+        _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL;
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         uint64_t usage, total = 0;
         char fb[FORMAT_BYTES_MAX];
@@ -2427,15 +2427,22 @@ static int clean_images(int argc, char *argv[], void *userdata) {
         unsigned c = 0;
         int r;
 
-        r = sd_bus_call_method(
+        r = sd_bus_message_new_method_call(
                         bus,
+                        &m,
                         "org.freedesktop.machine1",
                         "/org/freedesktop/machine1",
                         "org.freedesktop.machine1.Manager",
-                        "CleanPool",
-                        &error,
-                        &reply,
-                        "s", arg_all ? "all" : "hidden");
+                        "CleanPool");
+        if (r < 0)
+                return bus_log_create_error(r);
+
+        r = sd_bus_message_append(m, "s", arg_all ? "all" : "hidden");
+        if (r < 0)
+                return bus_log_create_error(r);
+
+        /* This is a slow operation, hence permit a longer time for completion. */
+        r = sd_bus_call(bus, m, USEC_INFINITY, &error, &reply);
         if (r < 0)
                 return log_error_errno(r, "Could not clean pool: %s", bus_error_message(&error, r));
 
diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c
index 31efa3695b..52ce83a185 100644
--- a/src/machine/machined-dbus.c
+++ b/src/machine/machined-dbus.c
@@ -29,6 +29,7 @@
 #include "bus-util.h"
 #include "cgroup-util.h"
 #include "fd-util.h"
+#include "fileio.h"
 #include "formats-util.h"
 #include "hostname-util.h"
 #include "image-dbus.h"
@@ -822,22 +823,106 @@ static int method_mark_image_read_only(sd_bus_message *message, void *userdata,
         return bus_image_method_mark_read_only(message, i, error);
 }
 
+static int clean_pool_done(Operation *operation, int ret, sd_bus_error *error) {
+        _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
+        _cleanup_fclose_ FILE *f = NULL;
+        bool success;
+        size_t n;
+        int r;
+
+        assert(operation);
+        assert(operation->extra_fd >= 0);
+
+        if (lseek(operation->extra_fd, 0, SEEK_SET) == (off_t) -1)
+                return -errno;
+
+        f = fdopen(operation->extra_fd, "re");
+        if (!f)
+                return -errno;
+
+        operation->extra_fd = -1;
+
+        /* The resulting temporary file starts with a boolean value that indicates success or not. */
+        errno = 0;
+        n = fread(&success, 1, sizeof(success), f);
+        if (n != sizeof(success))
+                return ret < 0 ? ret : (errno != 0 ? -errno : -EIO);
+
+        if (ret < 0) {
+                _cleanup_free_ char *name = NULL;
+
+                /* The clean-up operation failed. In this case the resulting temporary file should contain a boolean
+                 * set to false followed by the name of the failed image. Let's try to read this and use it for the
+                 * error message. If we can't read it, don't mind, and return the naked error. */
+
+                if (success) /* The resulting temporary file could not be updated, ignore it. */
+                        return ret;
+
+                r = read_nul_string(f, &name);
+                if (r < 0 || isempty(name)) /* Same here... */
+                        return ret;
+
+                return sd_bus_error_set_errnof(error, ret, "Failed to remove image %s: %m", name);
+        }
+
+        assert(success);
+
+        r = sd_bus_message_new_method_return(operation->message, &reply);
+        if (r < 0)
+                return r;
+
+        r = sd_bus_message_open_container(reply, 'a', "(st)");
+        if (r < 0)
+                return r;
+
+        /* On success the resulting temporary file will contain a list of image names that were removed followed by
+         * their size on disk. Let's read that and turn it into a bus message. */
+        for (;;) {
+                _cleanup_free_ char *name = NULL;
+                uint64_t size;
+
+                r = read_nul_string(f, &name);
+                if (r < 0)
+                        return r;
+                if (isempty(name)) /* reached the end */
+                        break;
+
+                errno = 0;
+                n = fread(&size, 1, sizeof(size), f);
+                if (n != sizeof(size))
+                        return errno != 0 ? -errno : -EIO;
+
+                r = sd_bus_message_append(reply, "(st)", name, size);
+                if (r < 0)
+                        return r;
+        }
+
+        r = sd_bus_message_close_container(reply);
+        if (r < 0)
+                return r;
+
+        return sd_bus_send(NULL, reply, NULL);
+}
+
 static int method_clean_pool(sd_bus_message *message, void *userdata, sd_bus_error *error) {
         enum {
                 REMOVE_ALL,
                 REMOVE_HIDDEN,
         } mode;
 
-        _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
-        _cleanup_(image_hashmap_freep) Hashmap *images = NULL;
+        _cleanup_close_pair_ int errno_pipe_fd[2] = { -1, -1 };
+        _cleanup_close_ int result_fd = -1;
         Manager *m = userdata;
-        Image *image;
+        Operation *operation;
         const char *mm;
-        Iterator i;
+        pid_t child;
         int r;
 
         assert(message);
 
+        if (m->n_operations >= OPERATIONS_MAX)
+                return sd_bus_error_setf(error, SD_BUS_ERROR_LIMITS_EXCEEDED, "Too many ongoing operations.");
+
         r = sd_bus_message_read(message, "s", &mm);
         if (r < 0)
                 return r;
@@ -863,50 +948,109 @@ static int method_clean_pool(sd_bus_message *message, void *userdata, sd_bus_err
         if (r == 0)
                 return 1; /* Will call us back */
 
-        images = hashmap_new(&string_hash_ops);
-        if (!images)
-                return -ENOMEM;
+        if (pipe2(errno_pipe_fd, O_CLOEXEC|O_NONBLOCK) < 0)
+                return sd_bus_error_set_errnof(error, errno, "Failed to create pipe: %m");
 
-        r = image_discover(images);
-        if (r < 0)
-                return r;
+        /* Create a temporary file we can dump information about deleted images into. We use a temporary file for this
+         * instead of a pipe or so, since this might grow quit large in theory and we don't want to process this
+         * continously */
+        result_fd = open_tmpfile_unlinkable("/tmp/", O_RDWR|O_CLOEXEC);
+        if (result_fd < 0)
+                return -errno;
 
-        r = sd_bus_message_new_method_return(message, &reply);
-        if (r < 0)
-                return r;
+        /* This might be a slow operation, run it asynchronously in a background process */
+        child = fork();
+        if (child < 0)
+                return sd_bus_error_set_errnof(error, errno, "Failed to fork(): %m");
 
-        r = sd_bus_message_open_container(reply, 'a', "(st)");
-        if (r < 0)
-                return r;
+        if (child == 0) {
+                _cleanup_(image_hashmap_freep) Hashmap *images = NULL;
+                bool success = true;
+                Image *image;
+                Iterator i;
+                ssize_t l;
 
-        HASHMAP_FOREACH(image, images, i) {
+                errno_pipe_fd[0] = safe_close(errno_pipe_fd[0]);
 
-                /* We can't remove vendor images (i.e. those in /usr) */
-                if (IMAGE_IS_VENDOR(image))
-                        continue;
+                images = hashmap_new(&string_hash_ops);
+                if (!images) {
+                        r = -ENOMEM;
+                        goto child_fail;
+                }
 
-                if (IMAGE_IS_HOST(image))
-                        continue;
+                r = image_discover(images);
+                if (r < 0)
+                        goto child_fail;
 
-                if (mode == REMOVE_HIDDEN && !IMAGE_IS_HIDDEN(image))
-                        continue;
+                l = write(result_fd, &success, sizeof(success));
+                if (l < 0) {
+                        r = -errno;
+                        goto child_fail;
+                }
 
-                r = image_remove(image);
-                if (r == -EBUSY) /* keep images that are currently being used. */
-                        continue;
-                if (r < 0)
-                        return sd_bus_error_set_errnof(error, r, "Failed to remove image %s: %m", image->name);
+                HASHMAP_FOREACH(image, images, i) {
 
-                r = sd_bus_message_append(reply, "(st)", image->name, image->usage_exclusive);
-                if (r < 0)
-                        return r;
+                        /* We can't remove vendor images (i.e. those in /usr) */
+                        if (IMAGE_IS_VENDOR(image))
+                                continue;
+
+                        if (IMAGE_IS_HOST(image))
+                                continue;
+
+                        if (mode == REMOVE_HIDDEN && !IMAGE_IS_HIDDEN(image))
+                                continue;
+
+                        r = image_remove(image);
+                        if (r == -EBUSY) /* keep images that are currently being used. */
+                                continue;
+                        if (r < 0) {
+                                /* If the operation failed, let's override everything we wrote, and instead write there at which image we failed. */
+                                success = false;
+                                (void) ftruncate(result_fd, 0);
+                                (void) lseek(result_fd, 0, SEEK_SET);
+                                (void) write(result_fd, &success, sizeof(success));
+                                (void) write(result_fd, image->name, strlen(image->name)+1);
+                                goto child_fail;
+                        }
+
+                        l = write(result_fd, image->name, strlen(image->name)+1);
+                        if (l < 0) {
+                                r = -errno;
+                                goto child_fail;
+                        }
+
+                        l = write(result_fd, &image->usage_exclusive, sizeof(image->usage_exclusive));
+                        if (l < 0) {
+                                r = -errno;
+                                goto child_fail;
+                        }
+                }
+
+                result_fd = safe_close(result_fd);
+                _exit(EXIT_SUCCESS);
+
+        child_fail:
+                (void) write(errno_pipe_fd[1], &r, sizeof(r));
+                _exit(EXIT_FAILURE);
         }
 
-        r = sd_bus_message_close_container(reply);
-        if (r < 0)
+        errno_pipe_fd[1] = safe_close(errno_pipe_fd[1]);
+
+        /* The clean-up might take a while, hence install a watch on the child and return */
+
+        r = operation_new(m, NULL, child, message, errno_pipe_fd[0], &operation);
+        if (r < 0) {
+                (void) sigkill_wait(child);
                 return r;
+        }
 
-        return sd_bus_send(NULL, reply, NULL);
+        operation->extra_fd = result_fd;
+        operation->done = clean_pool_done;
+
+        result_fd = -1;
+        errno_pipe_fd[0] = -1;
+
+        return 1;
 }
 
 static int method_set_pool_limit(sd_bus_message *message, void *userdata, sd_bus_error *error) {
diff --git a/src/machine/operation.c b/src/machine/operation.c
index e6ddc41a55..8f8321a8b3 100644
--- a/src/machine/operation.c
+++ b/src/machine/operation.c
@@ -41,18 +41,33 @@ static int operation_done(sd_event_source *s, const siginfo_t *si, void *userdat
                 goto fail;
         }
 
-        if (si->si_status != EXIT_SUCCESS) {
-                if (read(o->errno_fd, &r, sizeof(r)) == sizeof(r))
-                        r = sd_bus_error_set_errnof(&error, r, "%m");
-                else
-                        r = sd_bus_error_setf(&error, SD_BUS_ERROR_FAILED, "Child failed.");
-
+        if (si->si_status == EXIT_SUCCESS)
+                r = 0;
+        else if (read(o->errno_fd, &r, sizeof(r)) != sizeof(r)) { /* Try to acquire error code for failed operation */
+                r = sd_bus_error_setf(&error, SD_BUS_ERROR_FAILED, "Child failed.");
                 goto fail;
         }
 
-        r = sd_bus_reply_method_return(o->message, NULL);
-        if (r < 0)
-                log_error_errno(r, "Failed to reply to message: %m");
+        if (o->done) {
+                /* A completion routine is set for this operation, call it. */
+                r = o->done(o, r, &error);
+                if (r < 0) {
+                        if (!sd_bus_error_is_set(&error))
+                                sd_bus_error_set_errno(&error, r);
+
+                        goto fail;
+                }
+
+        } else {
+                /* The default default operaton when done is to simply return an error on failure or an empty success
+                 * message on success. */
+                if (r < 0)
+                        goto fail;
+
+                r = sd_bus_reply_method_return(o->message, NULL);
+                if (r < 0)
+                        log_error_errno(r, "Failed to reply to message: %m");
+        }
 
         operation_free(o);
         return 0;
@@ -66,7 +81,7 @@ fail:
         return 0;
 }
 
-int operation_new(Manager *manager, Machine *machine, pid_t child, sd_bus_message *message, int errno_fd) {
+int operation_new(Manager *manager, Machine *machine, pid_t child, sd_bus_message *message, int errno_fd, Operation **ret) {
         Operation *o;
         int r;
 
@@ -79,6 +94,8 @@ int operation_new(Manager *manager, Machine *machine, pid_t child, sd_bus_messag
         if (!o)
                 return -ENOMEM;
 
+        o->extra_fd = -1;
+
         r = sd_event_add_child(manager->event, &o->event_source, child, WEXITED, operation_done, o);
         if (r < 0) {
                 free(o);
@@ -102,6 +119,9 @@ int operation_new(Manager *manager, Machine *machine, pid_t child, sd_bus_messag
 
         /* At this point we took ownership of both the child and the errno file descriptor! */
 
+        if (ret)
+                *ret = o;
+
         return 0;
 }
 
@@ -112,6 +132,7 @@ Operation *operation_free(Operation *o) {
         sd_event_source_unref(o->event_source);
 
         safe_close(o->errno_fd);
+        safe_close(o->extra_fd);
 
         if (o->pid > 1)
                 (void) sigkill_wait(o->pid);
diff --git a/src/machine/operation.h b/src/machine/operation.h
index 7ca47bc3af..9831b123d7 100644
--- a/src/machine/operation.h
+++ b/src/machine/operation.h
@@ -38,10 +38,12 @@ struct Operation {
         pid_t pid;
         sd_bus_message *message;
         int errno_fd;
+        int extra_fd;
         sd_event_source *event_source;
+        int (*done)(Operation *o, int ret, sd_bus_error *error);
         LIST_FIELDS(Operation, operations);
         LIST_FIELDS(Operation, operations_by_machine);
 };
 
-int operation_new(Manager *manager, Machine *machine, pid_t child, sd_bus_message *message, int errno_fd);
+int operation_new(Manager *manager, Machine *machine, pid_t child, sd_bus_message *message, int errno_fd, Operation **ret);
 Operation *operation_free(Operation *o);
diff --git a/src/machine/org.freedesktop.machine1.conf b/src/machine/org.freedesktop.machine1.conf
index 9d40b90151..562b9d3cc0 100644
--- a/src/machine/org.freedesktop.machine1.conf
+++ b/src/machine/org.freedesktop.machine1.conf
@@ -118,6 +118,10 @@
 
                 <allow send_destination="org.freedesktop.machine1"
                        send_interface="org.freedesktop.machine1.Manager"
+                       send_member="CleanPool"/>
+
+                <allow send_destination="org.freedesktop.machine1"
+                       send_interface="org.freedesktop.machine1.Manager"
                        send_member="MapFromMachineUser"/>
 
                 <allow send_destination="org.freedesktop.machine1"