shared/install,systemctl,core: report offending file on installation error

Fixes #2191:

$ systemctl --root=/ enable sddm
Created symlink /etc/systemd/system/display-manager.service, pointing to /usr/lib/systemd/system/sddm.service.
$ sudo build/systemctl --root=/ enable gdm
Failed to enable unit, file /etc/systemd/system/display-manager.service already exists and is a symlink to /usr/lib/systemd/system/sddm.service.
$ sudo build/systemctl --root= enable sddm
$ sudo build/systemctl --root= enable gdm
Failed to enable unit: File /etc/systemd/system/display-manager.service already exists and is a symlink to /usr/lib/systemd/system/sddm.service.

(I tried a few different approaches to pass the error information back to the
caller. Adding a new parameter to hold the error results in a gigantic patch
and a lot of hassle to pass the args arounds. Adding this information to the
changes array is straightforward and can be more easily extended in the
future.)

In case local installation is performed, the full set of errors can be reported
and we do that. When running over dbus, only the first error is reported.

5 files changed, 205 insertions, 145 deletions

diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
index ddfde23028..d48b0ca69d 100644
--- a/src/core/dbus-manager.c
+++ b/src/core/dbus-manager.c
@@ -1585,15 +1585,19 @@ static int reply_unit_file_changes_and_free(
         if (r < 0)
                 goto fail;
 
-        for (i = 0; i < n_changes; i++) {
-                r = sd_bus_message_append(
-                                reply, "(sss)",
-                                unit_file_change_type_to_string(changes[i].type),
-                                changes[i].path,
-                                changes[i].source);
-                if (r < 0)
-                        goto fail;
-        }
+        for (i = 0; i < n_changes; i++)
+                if (changes[i].type >= 0) {
+                        const char *change = unit_file_change_type_to_string(changes[i].type);
+                        assert(change != NULL);
+
+                        r = sd_bus_message_append(
+                                        reply, "(sss)",
+                                        change,
+                                        changes[i].path,
+                                        changes[i].source);
+                        if (r < 0)
+                                goto fail;
+                }
 
         r = sd_bus_message_close_container(reply);
         if (r < 0)
@@ -1607,17 +1611,56 @@ fail:
         return r;
 }
 
-static int install_error(sd_bus_error *error, int c) {
+/* Create an error reply, using the error information from changes[]
+ * if possible, and fall back to generating an error from error code c.
+ * The error message only describes the first error.
+ *
+ * Coordinate with unit_file_dump_changes() in install.c.
+ */
+static int install_error(
+                sd_bus_error *error,
+                int c,
+                UnitFileChange *changes,
+                unsigned n_changes) {
+        int r;
+        unsigned i;
         assert(c < 0);
 
-        if (c == -ERFKILL)
-                return sd_bus_error_setf(error, BUS_ERROR_UNIT_MASKED, "Unit file is masked.");
-        if (c == -EADDRNOTAVAIL)
-                return sd_bus_error_setf(error, BUS_ERROR_UNIT_GENERATED, "Unit file is transient or generated.");
-        if (c == -ELOOP)
-                return sd_bus_error_setf(error, BUS_ERROR_UNIT_LINKED, "Refusing to operate on linked unit file.");
+        for (i = 0; i < n_changes; i++)
+                switch(changes[i].type) {
+                case 0 ... INT_MAX:
+                        continue;
+                case -EEXIST:
+                        if (changes[i].source)
+                                r = sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS,
+                                                      "File %s already exists and is a symlink to %s.",
+                                                      changes[i].path, changes[i].source);
+                        else
+                                r = sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS,
+                                                      "File %s already exists.",
+                                                      changes[i].path);
+                        goto found;
+                case -ERFKILL:
+                        r = sd_bus_error_setf(error, BUS_ERROR_UNIT_MASKED,
+                                              "Unit file %s is masked.", changes[i].path);
+                        goto found;
+                case -EADDRNOTAVAIL:
+                        r = sd_bus_error_setf(error, BUS_ERROR_UNIT_GENERATED,
+                                              "Unit %s is transient or generated.", changes[i].path);
+                        goto found;
+                case -ELOOP:
+                        r = sd_bus_error_setf(error, BUS_ERROR_UNIT_LINKED,
+                                              "Refusing to operate on linked unit file %s", changes[i].path);
+                        goto found;
+                default:
+                        r = sd_bus_error_set_errnof(error, changes[i].type, "File %s: %m", changes[i].path);
+                        goto found;
+                }
 
-        return c;
+        r = c;
+ found:
+        unit_file_changes_free(changes, n_changes);
+        return r;
 }
 
 static int method_enable_unit_files_generic(
@@ -1651,10 +1694,8 @@ static int method_enable_unit_files_generic(
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = call(m->unit_file_scope, runtime, NULL, l, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, carries_install_info ? r : -1, changes, n_changes);
 }
@@ -1719,10 +1760,8 @@ static int method_preset_unit_files_with_mode(sd_bus_message *message, void *use
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = unit_file_preset(m->unit_file_scope, runtime, NULL, l, mm, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, r, changes, n_changes);
 }
@@ -1757,10 +1796,8 @@ static int method_disable_unit_files_generic(
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = call(m->unit_file_scope, runtime, NULL, l, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
@@ -1794,10 +1831,8 @@ static int method_revert_unit_files(sd_bus_message *message, void *userdata, sd_
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = unit_file_revert(m->unit_file_scope, NULL, l, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
@@ -1827,10 +1862,8 @@ static int method_set_default_target(sd_bus_message *message, void *userdata, sd
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = unit_file_set_default(m->unit_file_scope, NULL, name, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
@@ -1869,10 +1902,8 @@ static int method_preset_all_unit_files(sd_bus_message *message, void *userdata,
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = unit_file_preset_all(m->unit_file_scope, runtime, NULL, mm, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
@@ -1908,10 +1939,8 @@ static int method_add_dependency_unit_files(sd_bus_message *message, void *userd
                 return -EINVAL;
 
         r = unit_file_add_dependency(m->unit_file_scope, runtime, NULL, l, target, dep, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
diff --git a/src/shared/bus-util.c b/src/shared/bus-util.c
index 677970b7f0..6a1877d8aa 100644
--- a/src/shared/bus-util.c
+++ b/src/shared/bus-util.c
@@ -2200,20 +2200,16 @@ int bus_deserialize_and_dump_unit_file_changes(sd_bus_message *m, bool quiet, Un
                 return bus_log_parse_error(r);
 
         while ((r = sd_bus_message_read(m, "(sss)", &type, &path, &source)) > 0) {
-                if (!quiet) {
-                        if (streq(type, "symlink"))
-                                log_info("Created symlink from %s to %s.", path, source);
-                        else if (streq(type, "unlink"))
-                                log_info("Removed symlink %s.", path);
-                        else if (streq(type, "masked"))
-                                log_info("Unit %s is masked, ignoring.", path);
-                        else
-                                log_notice("Manager reported unknown change type \"%s\" for %s.", type, path);
+                /* We expect only "success" changes to be sent over the bus.
+                   Hence, reject anything negative. */
+                UnitFileChangeType ch = unit_file_change_type_from_string(type);
+
+                if (ch < 0) {
+                        log_notice("Manager reported unknown change type \"%s\" for path \"%s\", ignoring.", type, path);
+                        continue;
                 }
 
-                r = unit_file_changes_add(changes, n_changes,
-                                          unit_file_change_type_from_string(type),
-                                          path, source);
+                r = unit_file_changes_add(changes, n_changes, ch, path, source);
                 if (r < 0)
                         return r;
         }
@@ -2224,6 +2220,7 @@ int bus_deserialize_and_dump_unit_file_changes(sd_bus_message *m, bool quiet, Un
         if (r < 0)
                 return bus_log_parse_error(r);
 
+        unit_file_dump_changes(0, NULL, *changes, *n_changes, false);
         return 0;
 }
 
diff --git a/src/shared/install.c b/src/shared/install.c
index 1522435f7f..febe33ed7b 100644
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -276,6 +276,70 @@ void unit_file_changes_free(UnitFileChange *changes, unsigned n_changes) {
         free(changes);
 }
 
+void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *changes, unsigned n_changes, bool quiet) {
+        unsigned i;
+        bool logged = false;
+
+        assert(changes || n_changes == 0);
+        /* If verb is not specified, errors are not allowed! */
+        assert(verb || r >= 0);
+
+        for (i = 0; i < n_changes; i++) {
+                assert(verb || changes[i].type >= 0);
+
+                switch(changes[i].type) {
+                case UNIT_FILE_SYMLINK:
+                        if (!quiet)
+                                log_info("Created symlink %s, pointing to %s.", changes[i].path, changes[i].source);
+                        break;
+                case UNIT_FILE_UNLINK:
+                        if (!quiet)
+                                log_info("Removed %s.", changes[i].path);
+                        break;
+                case UNIT_FILE_IS_MASKED:
+                        if (!quiet)
+                                log_info("Unit %s is masked, ignoring.", changes[i].path);
+                        break;
+                case -EEXIST:
+                        if (changes[i].source)
+                                log_error_errno(changes[i].type,
+                                                "Failed to %s unit, file %s already exists and is a symlink to %s.",
+                                                verb, changes[i].path, changes[i].source);
+                        else
+                                log_error_errno(changes[i].type,
+                                                "Failed to %s unit, file %s already exists.",
+                                                verb, changes[i].path);
+                        logged = true;
+                        break;
+                case -ERFKILL:
+                        log_error_errno(changes[i].type, "Failed to %s unit, unit %s is masked.",
+                                        verb, changes[i].path);
+                        logged = true;
+                        break;
+                case -EADDRNOTAVAIL:
+                        log_error_errno(changes[i].type, "Failed to %s unit, unit %s is transient or generated.",
+                                        verb, changes[i].path);
+                        logged = true;
+                        break;
+                case -ELOOP:
+                        log_error_errno(changes[i].type, "Failed to %s unit, refusing to operate on linked unit file %s",
+                                        verb, changes[i].path);
+                        logged = true;
+                        break;
+                default:
+                        assert(changes[i].type < 0);
+                        log_error_errno(changes[i].type, "Failed to %s unit, file %s: %m.",
+                                        verb, changes[i].path);
+                        logged = true;
+                }
+        }
+
+        if (r < 0 && !logged)
+                log_error_errno(r, "Failed to %s: %m.", verb);
+}
+
+
+
 static int create_symlink(
                 const char *old_path,
                 const char *new_path,
@@ -300,8 +364,10 @@ static int create_symlink(
                 return 1;
         }
 
-        if (errno != EEXIST)
+        if (errno != EEXIST) {
+                unit_file_changes_add(changes, n_changes, -errno, new_path, NULL);
                 return -errno;
+        }
 
         r = readlink_malloc(new_path, &dest);
         if (r < 0)
@@ -310,8 +376,10 @@ static int create_symlink(
         if (path_equal(dest, old_path))
                 return 0;
 
-        if (!force)
+        if (!force) {
+                unit_file_changes_add(changes, n_changes, -EEXIST, new_path, dest);
                 return -EEXIST;
+        }
 
         r = symlink_atomic(old_path, new_path);
         if (r < 0)
@@ -421,6 +489,7 @@ static int remove_marked_symlinks_fd(
                         p = path_make_absolute(de->d_name, path);
                         if (!p)
                                 return -ENOMEM;
+                        path_kill_slashes(p);
 
                         q = readlink_malloc(p, &dest);
                         if (q == -ENOENT)
@@ -444,10 +513,10 @@ static int remove_marked_symlinks_fd(
                         if (unlinkat(fd, de->d_name, 0) < 0 && errno != ENOENT) {
                                 if (r == 0)
                                         r = -errno;
+                                unit_file_changes_add(changes, n_changes, -errno, p, NULL);
                                 continue;
                         }
 
-                        path_kill_slashes(p);
                         (void) rmdir_parents(p, config_path);
 
                         unit_file_changes_add(changes, n_changes, UNIT_FILE_UNLINK, p, NULL);
@@ -745,19 +814,26 @@ static UnitFileInstallInfo *install_info_find(InstallContext *c, const char *nam
         return ordered_hashmap_get(c->will_process, name);
 }
 
-static int install_info_may_process(UnitFileInstallInfo *i, const LookupPaths *paths) {
+static int install_info_may_process(
+                UnitFileInstallInfo *i,
+                const LookupPaths *paths,
+                UnitFileChange **changes,
+                unsigned *n_changes) {
         assert(i);
         assert(paths);
 
         /* Checks whether the loaded unit file is one we should process, or is masked, transient or generated and thus
          * not subject to enable/disable operations. */
 
-        if (i->type == UNIT_FILE_TYPE_MASKED)
+        if (i->type == UNIT_FILE_TYPE_MASKED) {
+                unit_file_changes_add(changes, n_changes, -ERFKILL, i->path, NULL);
                 return -ERFKILL;
-        if (path_is_generator(paths, i->path))
-                return -EADDRNOTAVAIL;
-        if (path_is_transient(paths, i->path))
+        }
+        if (path_is_generator(paths, i->path) ||
+            path_is_transient(paths, i->path)) {
+                unit_file_changes_add(changes, n_changes, -EADDRNOTAVAIL, i->path, NULL);
                 return -EADDRNOTAVAIL;
+        }
 
         return 0;
 }
@@ -1637,8 +1713,11 @@ int unit_file_unmask(
                         return -ENOMEM;
 
                 if (unlink(path) < 0) {
-                        if (errno != ENOENT && r >= 0)
-                                r = -errno;
+                        if (errno != ENOENT) {
+                                if (r >= 0)
+                                        r = -errno;
+                                unit_file_changes_add(changes, n_changes, -errno, path, NULL);
+                        }
 
                         continue;
                 }
@@ -1953,7 +2032,7 @@ int unit_file_add_dependency(
         r = install_info_discover(scope, &c, &paths, target, SEARCH_FOLLOW_CONFIG_SYMLINKS, &target_info);
         if (r < 0)
                 return r;
-        r = install_info_may_process(target_info, &paths);
+        r = install_info_may_process(target_info, &paths, changes, n_changes);
         if (r < 0)
                 return r;
 
@@ -1965,7 +2044,7 @@ int unit_file_add_dependency(
                 r = install_info_discover(scope, &c, &paths, *f, SEARCH_FOLLOW_CONFIG_SYMLINKS, &i);
                 if (r < 0)
                         return r;
-                r = install_info_may_process(i, &paths);
+                r = install_info_may_process(i, &paths, changes, n_changes);
                 if (r < 0)
                         return r;
 
@@ -2018,7 +2097,7 @@ int unit_file_enable(
                 r = install_info_discover(scope, &c, &paths, *f, SEARCH_LOAD, &i);
                 if (r < 0)
                         return r;
-                r = install_info_may_process(i, &paths);
+                r = install_info_may_process(i, &paths, changes, n_changes);
                 if (r < 0)
                         return r;
 
@@ -2131,7 +2210,7 @@ int unit_file_set_default(
         r = install_info_discover(scope, &c, &paths, name, 0, &i);
         if (r < 0)
                 return r;
-        r = install_info_may_process(i, &paths);
+        r = install_info_may_process(i, &paths, changes, n_changes);
         if (r < 0)
                 return r;
 
@@ -2163,7 +2242,7 @@ int unit_file_get_default(
         r = install_info_discover(scope, &c, &paths, SPECIAL_DEFAULT_TARGET, SEARCH_FOLLOW_CONFIG_SYMLINKS, &i);
         if (r < 0)
                 return r;
-        r = install_info_may_process(i, &paths);
+        r = install_info_may_process(i, &paths, NULL, 0);
         if (r < 0)
                 return r;
 
@@ -2425,7 +2504,9 @@ static int preset_prepare_one(
                 InstallContext *minus,
                 LookupPaths *paths,
                 UnitFilePresetMode mode,
-                const char *name) {
+                const char *name,
+                UnitFileChange **changes,
+                unsigned *n_changes) {
 
         UnitFileInstallInfo *i;
         int r;
@@ -2443,7 +2524,7 @@ static int preset_prepare_one(
                 if (r < 0)
                         return r;
 
-                r = install_info_may_process(i, paths);
+                r = install_info_may_process(i, paths, changes, n_changes);
                 if (r < 0)
                         return r;
         } else
@@ -2482,7 +2563,7 @@ int unit_file_preset(
                 if (!unit_name_is_valid(*i, UNIT_NAME_ANY))
                         return -EINVAL;
 
-                r = preset_prepare_one(scope, &plus, &minus, &paths, mode, *i);
+                r = preset_prepare_one(scope, &plus, &minus, &paths, mode, *i, changes, n_changes);
                 if (r < 0)
                         return r;
         }
@@ -2537,7 +2618,8 @@ int unit_file_preset_all(
                         if (!IN_SET(de->d_type, DT_LNK, DT_REG))
                                 continue;
 
-                        r = preset_prepare_one(scope, &plus, &minus, &paths, mode, de->d_name);
+                        /* we don't pass changes[] in, because we want to handle errors on our own */
+                        r = preset_prepare_one(scope, &plus, &minus, &paths, mode, de->d_name, NULL, 0);
                         if (r == -ERFKILL)
                                 r = unit_file_changes_add(changes, n_changes,
                                                           UNIT_FILE_IS_MASKED, de->d_name, NULL);
diff --git a/src/shared/install.h b/src/shared/install.h
index 219b48f428..82c62095d5 100644
--- a/src/shared/install.h
+++ b/src/shared/install.h
@@ -77,8 +77,12 @@ enum UnitFileChangeType {
         _UNIT_FILE_CHANGE_TYPE_INVALID = -1
 };
 
+/* type can either one of the UnitFileChangeTypes listed above, or a negative error.
+ * If source is specified, it should be the contents of the path symlink.
+ * In case of an error, source should be the existing symlink contents or NULL
+ */
 struct UnitFileChange {
-        UnitFileChangeType type;
+        int type; /* UnitFileChangeType or bust */
         char *path;
         char *source;
 };
@@ -233,6 +237,7 @@ Hashmap* unit_file_list_free(Hashmap *h);
 
 int unit_file_changes_add(UnitFileChange **changes, unsigned *n_changes, UnitFileChangeType type, const char *path, const char *source);
 void unit_file_changes_free(UnitFileChange *changes, unsigned n_changes);
+void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *changes, unsigned n_changes, bool quiet);
 
 int unit_file_query_preset(UnitFileScope scope, const char *root_dir, const char *name);
 
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 5f87e4a97f..cfdc851a6d 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -1984,27 +1984,6 @@ static int get_default(int argc, char *argv[], void *userdata) {
         return 0;
 }
 
-static void dump_unit_file_changes(const UnitFileChange *changes, unsigned n_changes) {
-        unsigned i;
-
-        assert(changes || n_changes == 0);
-
-        for (i = 0; i < n_changes; i++)
-                switch(changes[i].type) {
-                case UNIT_FILE_SYMLINK:
-                        log_info("Created symlink %s, pointing to %s.", changes[i].path, changes[i].source);
-                        break;
-                case UNIT_FILE_UNLINK:
-                        log_info("Removed %s.", changes[i].path);
-                        break;
-                case UNIT_FILE_IS_MASKED:
-                        log_info("Unit %s is masked, ignoring.", changes[i].path);
-                        break;
-                default:
-                        assert_not_reached("bad change type");
-                }
-}
-
 static int set_default(int argc, char *argv[], void *userdata) {
         _cleanup_free_ char *unit = NULL;
         int r;
@@ -2021,14 +2000,9 @@ static int set_default(int argc, char *argv[], void *userdata) {
                 unsigned n_changes = 0;
 
                 r = unit_file_set_default(arg_scope, arg_root, unit, true, &changes, &n_changes);
-                if (r < 0)
-                        return log_error_errno(r, "Failed to set default target: %m");
-
-                if (!arg_quiet)
-                        dump_unit_file_changes(changes, n_changes);
-
+                unit_file_dump_changes(r, "set default", changes, n_changes, arg_quiet);
                 unit_file_changes_free(changes, n_changes);
-                r = 0;
+                return r;
         } else {
                 _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
                 _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
@@ -3117,7 +3091,7 @@ static int set_exit_code(uint8_t code) {
                         NULL,
                         "y", code);
         if (r < 0)
-                return log_error_errno(r, "Failed to execute operation: %s", bus_error_message(&error, r));
+                return log_error_errno(r, "Failed to set exit code: %s", bus_error_message(&error, r));
 
         return 0;
 }
@@ -4967,7 +4941,7 @@ static int daemon_reload(int argc, char *argv[], void *userdata) {
                  * reply */
                 r = 0;
         else if (r < 0)
-                return log_error_errno(r, "Failed to execute operation: %s", bus_error_message(&error, r));
+                return log_error_errno(r, "Failed to reload daemon: %s", bus_error_message(&error, r));
 
         return r < 0 ? r : 0;
 }
@@ -5450,18 +5424,9 @@ static int enable_unit(int argc, char *argv[], void *userdata) {
                 else
                         assert_not_reached("Unknown verb");
 
-                if (r == -ERFKILL)
-                        return log_error_errno(r, "Unit file is masked.");
-                if (r == -EADDRNOTAVAIL)
-                        return log_error_errno(r, "Unit file is transient or generated.");
-                if (r == -ELOOP)
-                        return log_error_errno(r, "Refusing to operate on linked unit file.");
+                unit_file_dump_changes(r, verb, changes, n_changes, arg_quiet);
                 if (r < 0)
-                        return log_error_errno(r, "Operation failed: %m");
-
-                if (!arg_quiet)
-                        dump_unit_file_changes(changes, n_changes);
-
+                        return r;
                 r = 0;
         } else {
                 _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL, *m = NULL;
@@ -5542,7 +5507,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) {
 
                 r = sd_bus_call(bus, m, 0, &error, &reply);
                 if (r < 0)
-                        return log_error_errno(r, "Failed to execute operation: %s", bus_error_message(&error, r));
+                        return log_error_errno(r, "Failed to %s unit: %s", verb, bus_error_message(&error, r));
 
                 if (expect_carries_install_info) {
                         r = sd_bus_message_read(reply, "b", &carries_install_info);
@@ -5625,18 +5590,9 @@ static int add_dependency(int argc, char *argv[], void *userdata) {
                 unsigned n_changes = 0;
 
                 r = unit_file_add_dependency(arg_scope, arg_runtime, arg_root, names, target, dep, arg_force, &changes, &n_changes);
-                if (r == -ERFKILL)
-                        return log_error_errno(r, "Unit file is masked.");
-                if (r == -EADDRNOTAVAIL)
-                        return log_error_errno(r, "Unit file is transient or generated.");
-                if (r < 0)
-                        return log_error_errno(r, "Can't add dependency: %m");
-
-                if (!arg_quiet)
-                        dump_unit_file_changes(changes, n_changes);
-
+                unit_file_dump_changes(r, "add dependency on", changes, n_changes, arg_quiet);
                 unit_file_changes_free(changes, n_changes);
-
+                return r;
         } else {
                 _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL, *m = NULL;
                 _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
@@ -5668,19 +5624,16 @@ static int add_dependency(int argc, char *argv[], void *userdata) {
 
                 r = sd_bus_call(bus, m, 0, &error, &reply);
                 if (r < 0)
-                        return log_error_errno(r, "Failed to execute operation: %s", bus_error_message(&error, r));
+                        return log_error_errno(r, "Failed to add dependency: %s", bus_error_message(&error, r));
 
                 r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, NULL, NULL);
                 if (r < 0)
                         return r;
 
-                if (!arg_no_reload)
-                        r = daemon_reload(argc, argv, userdata);
-                else
-                        r = 0;
+                if (arg_no_reload)
+                        return 0;
+                return daemon_reload(argc, argv, userdata);
         }
-
-        return r;
 }
 
 static int preset_all(int argc, char *argv[], void *userdata) {
@@ -5691,13 +5644,7 @@ static int preset_all(int argc, char *argv[], void *userdata) {
                 unsigned n_changes = 0;
 
                 r = unit_file_preset_all(arg_scope, arg_runtime, arg_root, arg_preset_mode, arg_force, &changes, &n_changes);
-                if (r < 0)
-                        log_error_errno(r, "Operation failed: %m");
-                else {
-                        if (!arg_quiet)
-                                dump_unit_file_changes(changes, n_changes);
-                }
-
+                unit_file_dump_changes(r, "preset", changes, n_changes, arg_quiet);
                 unit_file_changes_free(changes, n_changes);
                 return r;
         } else {
@@ -5724,7 +5671,7 @@ static int preset_all(int argc, char *argv[], void *userdata) {
                                 arg_runtime,
                                 arg_force);
                 if (r < 0)
-                        return log_error_errno(r, "Failed to execute operation: %s", bus_error_message(&error, r));
+                        return log_error_errno(r, "Failed to preset all units: %s", bus_error_message(&error, r));
 
                 r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, NULL, NULL);
                 if (r < 0)