Merge pull request #3055 from keszybz/preset-fixes

Another bunch of improvements to the installation code

8 files changed, 361 insertions, 221 deletions

diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
index 9a913a6c91..d48b0ca69d 100644
--- a/src/core/dbus-manager.c
+++ b/src/core/dbus-manager.c
@@ -1565,13 +1565,11 @@ static int reply_unit_file_changes_and_free(
         unsigned i;
         int r;
 
-        for (i = 0; i < n_changes; i++)
-                if (unit_file_change_is_modification(changes[i].type)) {
-                        r = bus_foreach_bus(m, NULL, send_unit_files_changed, NULL);
-                        if (r < 0)
-                                log_debug_errno(r, "Failed to send UnitFilesChanged signal: %m");
-                        break;
-                }
+        if (unit_file_changes_have_modification(changes, n_changes)) {
+                r = bus_foreach_bus(m, NULL, send_unit_files_changed, NULL);
+                if (r < 0)
+                        log_debug_errno(r, "Failed to send UnitFilesChanged signal: %m");
+        }
 
         r = sd_bus_message_new_method_return(message, &reply);
         if (r < 0)
@@ -1587,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)
@@ -1609,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(
@@ -1653,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);
 }
@@ -1721,18 +1760,16 @@ 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);
 }
 
 static int method_disable_unit_files_generic(
                 sd_bus_message *message,
-                Manager *m, const
-                char *verb,
+                Manager *m,
+                const char *verb,
                 int (*call)(UnitFileScope scope, bool runtime, const char *root_dir, char *files[], UnitFileChange **changes, unsigned *n_changes),
                 sd_bus_error *error) {
 
@@ -1759,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);
 }
@@ -1796,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);
 }
@@ -1829,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);
 }
@@ -1871,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);
 }
@@ -1885,8 +1914,7 @@ static int method_add_dependency_unit_files(sd_bus_message *message, void *userd
         UnitFileChange *changes = NULL;
         unsigned n_changes = 0;
         int runtime, force, r;
-        char *target;
-        char *type;
+        char *target, *type;
         UnitDependency dep;
 
         assert(message);
@@ -1911,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/run/run.c b/src/run/run.c
index f92a7f4e2e..c0aa2c5924 100644
--- a/src/run/run.c
+++ b/src/run/run.c
@@ -125,7 +125,6 @@ static int parse_argv(int argc, char *argv[]) {
 
         enum {
                 ARG_VERSION = 0x100,
-                ARG_NO_ASK_PASSWORD,
                 ARG_USER,
                 ARG_SYSTEM,
                 ARG_SCOPE,
@@ -133,12 +132,11 @@ static int parse_argv(int argc, char *argv[]) {
                 ARG_DESCRIPTION,
                 ARG_SLICE,
                 ARG_SEND_SIGHUP,
+                ARG_SERVICE_TYPE,
                 ARG_EXEC_USER,
                 ARG_EXEC_GROUP,
-                ARG_SERVICE_TYPE,
                 ARG_NICE,
                 ARG_SETENV,
-                ARG_TTY,
                 ARG_ON_ACTIVE,
                 ARG_ON_BOOT,
                 ARG_ON_STARTUP,
@@ -147,6 +145,7 @@ static int parse_argv(int argc, char *argv[]) {
                 ARG_ON_CALENDAR,
                 ARG_TIMER_PROPERTY,
                 ARG_NO_BLOCK,
+                ARG_NO_ASK_PASSWORD,
         };
 
         static const struct option options[] = {
@@ -168,7 +167,8 @@ static int parse_argv(int argc, char *argv[]) {
                 { "nice",              required_argument, NULL, ARG_NICE             },
                 { "setenv",            required_argument, NULL, ARG_SETENV           },
                 { "property",          required_argument, NULL, 'p'                  },
-                { "tty",               no_argument,       NULL, 't'                  },
+                { "tty",               no_argument,       NULL, 't'                  }, /* deprecated */
+                { "pty",               no_argument,       NULL, 't'                  },
                 { "quiet",             no_argument,       NULL, 'q'                  },
                 { "on-active",         required_argument, NULL, ARG_ON_ACTIVE        },
                 { "on-boot",           required_argument, NULL, ARG_ON_BOOT          },
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 10c724edbd..71012eafb4 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)
@@ -434,8 +503,7 @@ static int remove_marked_symlinks_fd(
                         /* We remove all links pointing to a file or path that is marked, as well as all files sharing
                          * the same name as a file that is marked. */
 
-                        found =
-                                set_contains(remove_symlinks_to, dest) ||
+                        found = set_contains(remove_symlinks_to, dest) ||
                                 set_contains(remove_symlinks_to, basename(dest)) ||
                                 set_contains(remove_symlinks_to, de->d_name);
 
@@ -445,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);
@@ -746,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;
 }
@@ -1391,8 +1466,10 @@ static int install_info_symlink_link(
         assert(i->path);
 
         r = in_search_path(paths, i->path);
-        if (r != 0)
+        if (r < 0)
                 return r;
+        if (r > 0)
+                return 0;
 
         path = strjoin(config_path, "/", i->name, NULL);
         if (!path)
@@ -1431,7 +1508,8 @@ static int install_info_apply(
                 r = q;
 
         q = install_info_symlink_link(i, paths, config_path, force, changes, n_changes);
-        if (r == 0)
+        /* Do not count links to the unit file towards the "carries_install_info" count */
+        if (r == 0 && q < 0)
                 r = q;
 
         return r;
@@ -1481,7 +1559,7 @@ static int install_context_apply(
                         if (q < 0)
                                 r = q;
                         else
-                                r+= q;
+                                r += q;
                 }
         }
 
@@ -1638,8 +1716,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;
                 }
@@ -1954,7 +2035,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;
 
@@ -1966,7 +2047,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;
 
@@ -2019,7 +2100,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;
 
@@ -2132,7 +2213,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;
 
@@ -2164,7 +2245,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;
 
@@ -2413,7 +2494,7 @@ static int execute_preset(
                         if (q < 0)
                                 r = q;
                         else
-                                r+= q;
+                                r += q;
                 }
         }
 
@@ -2426,7 +2507,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;
@@ -2444,7 +2527,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
@@ -2483,7 +2566,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;
         }
@@ -2538,7 +2621,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 6b760def9b..4133faffa2 100644
--- a/src/shared/install.h
+++ b/src/shared/install.h
@@ -74,19 +74,27 @@ enum UnitFileChangeType {
         UNIT_FILE_UNLINK,
         UNIT_FILE_IS_MASKED,
         _UNIT_FILE_CHANGE_TYPE_MAX,
-        _UNIT_FILE_CHANGE_TYPE_INVALID = -1
+        _UNIT_FILE_CHANGE_INVALID = INT_MIN
 };
 
-static inline bool unit_file_change_is_modification(UnitFileChangeType type) {
-        return IN_SET(type, UNIT_FILE_SYMLINK, UNIT_FILE_UNLINK);
-}
-
+/* 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;
 };
 
+static inline bool unit_file_changes_have_modification(const UnitFileChange* changes, unsigned n_changes) {
+        unsigned i;
+        for (i = 0; i < n_changes; i++)
+                if (IN_SET(changes[i].type, UNIT_FILE_SYMLINK, UNIT_FILE_UNLINK))
+                        return true;
+        return false;
+}
+
 struct UnitFileList {
         char *path;
         UnitFileState state;
@@ -130,18 +138,96 @@ static inline bool UNIT_FILE_INSTALL_INFO_HAS_ALSO(UnitFileInstallInfo *i) {
         return !strv_isempty(i->also);
 }
 
-int unit_file_enable(UnitFileScope scope, bool runtime, const char *root_dir, char **files, bool force, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_disable(UnitFileScope scope, bool runtime, const char *root_dir, char **files, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_reenable(UnitFileScope scope, bool runtime, const char *root_dir, char **files, bool force, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_preset(UnitFileScope scope, bool runtime, const char *root_dir, char **files, UnitFilePresetMode mode, bool force, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_preset_all(UnitFileScope scope, bool runtime, const char *root_dir, UnitFilePresetMode mode, bool force, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_mask(UnitFileScope scope, bool runtime, const char *root_dir, char **files, bool force, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_unmask(UnitFileScope scope, bool runtime, const char *root_dir, char **files, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_link(UnitFileScope scope, bool runtime, const char *root_dir, char **files, bool force, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_revert(UnitFileScope scope, const char *root_dir, char **files, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_set_default(UnitFileScope scope, const char *root_dir, const char *file, bool force, UnitFileChange **changes, unsigned *n_changes);
-int unit_file_get_default(UnitFileScope scope, const char *root_dir, char **name);
-int unit_file_add_dependency(UnitFileScope scope, bool runtime, const char *root_dir, char **files, const char *target, UnitDependency dep, bool force, UnitFileChange **changes, unsigned *n_changes);
+int unit_file_enable(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                char **files,
+                bool force,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_disable(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                char **files,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_reenable(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                char **files,
+                bool force,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_preset(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                char **files,
+                UnitFilePresetMode mode,
+                bool force,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_preset_all(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                UnitFilePresetMode mode,
+                bool force,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_mask(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                char **files,
+                bool force,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_unmask(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                char **files,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_link(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                char **files,
+                bool force,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_revert(
+                UnitFileScope scope,
+                const char *root_dir,
+                char **files,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_set_default(
+                UnitFileScope scope,
+                const char *root_dir,
+                const char *file,
+                bool force,
+                UnitFileChange **changes,
+                unsigned *n_changes);
+int unit_file_get_default(
+                UnitFileScope scope,
+                const char *root_dir,
+                char **name);
+int unit_file_add_dependency(
+                UnitFileScope scope,
+                bool runtime,
+                const char *root_dir,
+                char **files,
+                const char *target,
+                UnitDependency dep,
+                bool force,
+                UnitFileChange **changes,
+                unsigned *n_changes);
 
 int unit_file_get_state(UnitFileScope scope, const char *root_dir, const char *filename, UnitFileState *ret);
 int unit_file_exists(UnitFileScope scope, const LookupPaths *paths, const char *name);
@@ -151,11 +237,13 @@ 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);
 
 const char *unit_file_state_to_string(UnitFileState s) _const_;
 UnitFileState unit_file_state_from_string(const char *s) _pure_;
+/* from_string conversion is unreliable because of the overlap between -EPERM and -1 for error. */
 
 const char *unit_file_change_type_to_string(UnitFileChangeType s) _const_;
 UnitFileChangeType unit_file_change_type_from_string(const char *s) _pure_;
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 0c64cbab80..115c00ea9c 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);
@@ -5562,15 +5527,18 @@ static int enable_unit(int argc, char *argv[], void *userdata) {
         }
 
         if (carries_install_info == 0)
-                log_warning("The unit files have no [Install] section. They are not meant to be enabled\n"
-                            "using systemctl.\n"
+                log_warning("The unit files have no installation config (WantedBy, RequiredBy, Also, Alias\n"
+                            "settings in the [Install] section, and DefaultInstance for template units).\n"
+                            "This means they are not meant to be enabled using systemctl.\n"
                             "Possible reasons for having this kind of units are:\n"
                             "1) A unit may be statically enabled by being symlinked from another unit's\n"
                             "   .wants/ or .requires/ directory.\n"
                             "2) A unit's purpose may be to act as a helper for some other unit which has\n"
                             "   a requirement dependency on it.\n"
                             "3) A unit may be started when needed via activation (socket, path, timer,\n"
-                            "   D-Bus, udev, scripted systemctl call, ...).\n");
+                            "   D-Bus, udev, scripted systemctl call, ...).\n"
+                            "4) In case of template units, the unit is meant to be enabled with some\n"
+                            "   instance name specified.");
 
         if (arg_now && n_changes > 0 && STR_IN_SET(argv[0], "enable", "disable", "mask")) {
                 char *new_args[n_changes + 2];
@@ -5625,18 +5593,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,39 +5627,29 @@ 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) {
-        UnitFileChange *changes = NULL;
-        unsigned n_changes = 0;
         int r;
 
         if (install_client_side()) {
+                UnitFileChange *changes = NULL;
+                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");
-                        goto finish;
-                }
-
-                if (!arg_quiet)
-                        dump_unit_file_changes(changes, n_changes);
-
-                r = 0;
-
+                unit_file_dump_changes(r, "preset", changes, n_changes, arg_quiet);
+                unit_file_changes_free(changes, n_changes);
+                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;
@@ -5725,22 +5674,16 @@ 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)
                         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);
         }
-
-finish:
-        unit_file_changes_free(changes, n_changes);
-
-        return r;
 }
 
 static int unit_is_enabled(int argc, char *argv[], void *userdata) {
diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c
index 998c345a1a..2d73c9743b 100644
--- a/src/test/test-install-root.c
+++ b/src/test/test-install-root.c
@@ -107,7 +107,7 @@ static void test_basic_mask_and_enable(const char *root) {
         assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "d.service", &state) >= 0 && state == UNIT_FILE_ENABLED);
 
         /* Enabling it again should succeed but be a NOP */
-        assert_se(unit_file_enable(UNIT_FILE_SYSTEM, false, root, STRV_MAKE("a.service"), false, &changes, &n_changes) == 1);
+        assert_se(unit_file_enable(UNIT_FILE_SYSTEM, false, root, STRV_MAKE("a.service"), false, &changes, &n_changes) >= 0);
         assert_se(n_changes == 0);
         unit_file_changes_free(changes, n_changes);
         changes = NULL; n_changes = 0;
diff --git a/src/test/test-install.c b/src/test/test-install.c
index 874d617621..50315c1d9a 100644
--- a/src/test/test-install.c
+++ b/src/test/test-install.c
@@ -46,6 +46,9 @@ int main(int argc, char* argv[]) {
         unsigned n_changes = 0;
         UnitFileState state = 0;
 
+        log_set_max_level(LOG_DEBUG);
+        log_parse_environment();
+
         h = hashmap_new(&string_hash_ops);
         r = unit_file_get_list(UNIT_FILE_SYSTEM, NULL, h);
         assert_se(r == 0);
@@ -65,12 +68,12 @@ int main(int argc, char* argv[]) {
 
         unit_file_list_free(h);
 
-        log_error("enable");
+        log_info("/*** enable **/");
 
         r = unit_file_enable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
 
-        log_error("enable2");
+        log_info("/*** enable2 **/");
 
         r = unit_file_enable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
@@ -82,8 +85,7 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_ENABLED);
 
-        log_error("disable");
-
+        log_info("/*** disable ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -97,13 +99,13 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_DISABLED);
 
-        log_error("mask");
+        log_info("/*** mask ***/");
         changes = NULL;
         n_changes = 0;
 
         r = unit_file_mask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
-        log_error("mask2");
+        log_info("/*** mask2 ***/");
         r = unit_file_mask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, false, &changes, &n_changes);
         assert_se(r >= 0);
 
@@ -114,13 +116,13 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_MASKED);
 
-        log_error("unmask");
+        log_info("/*** unmask ***/");
         changes = NULL;
         n_changes = 0;
 
         r = unit_file_unmask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
-        log_error("unmask2");
+        log_info("/*** unmask2 ***/");
         r = unit_file_unmask(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
 
@@ -131,7 +133,7 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_DISABLED);
 
-        log_error("mask");
+        log_info("/*** mask ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -145,13 +147,13 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_MASKED);
 
-        log_error("disable");
+        log_info("/*** disable ***/");
         changes = NULL;
         n_changes = 0;
 
         r = unit_file_disable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
-        log_error("disable2");
+        log_info("/*** disable2 ***/");
         r = unit_file_disable(UNIT_FILE_SYSTEM, false, NULL, (char**) files, &changes, &n_changes);
         assert_se(r >= 0);
 
@@ -162,7 +164,7 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_MASKED);
 
-        log_error("umask");
+        log_info("/*** umask ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -176,7 +178,7 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_DISABLED);
 
-        log_error("enable files2");
+        log_info("/*** enable files2 ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -190,7 +192,7 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_ENABLED);
 
-        log_error("disable files2");
+        log_info("/*** disable files2 ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -203,7 +205,7 @@ int main(int argc, char* argv[]) {
         r = unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0]), &state);
         assert_se(r < 0);
 
-        log_error("link files2");
+        log_info("/*** link files2 ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -217,7 +219,7 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_LINKED);
 
-        log_error("disable files2");
+        log_info("/*** disable files2 ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -230,7 +232,7 @@ int main(int argc, char* argv[]) {
         r = unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0]), &state);
         assert_se(r < 0);
 
-        log_error("link files2");
+        log_info("/*** link files2 ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -244,7 +246,7 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_LINKED);
 
-        log_error("reenable files2");
+        log_info("/*** reenable files2 ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -258,7 +260,7 @@ int main(int argc, char* argv[]) {
         assert_se(r >= 0);
         assert_se(state == UNIT_FILE_ENABLED);
 
-        log_error("disable files2");
+        log_info("/*** disable files2 ***/");
         changes = NULL;
         n_changes = 0;
 
@@ -270,7 +272,7 @@ int main(int argc, char* argv[]) {
 
         r = unit_file_get_state(UNIT_FILE_SYSTEM, NULL, basename(files2[0]), &state);
         assert_se(r < 0);
-        log_error("preset files");
+        log_info("/*** preset files ***/");
         changes = NULL;
         n_changes = 0;