Merge pull request #3084 from keszybz/preset-fixes

Nicer error message is symlinking chokes on an existing file 

3 files changed, 38 insertions, 42 deletions

diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
index d48b0ca69d..0c86791fe3 100644
--- a/src/core/dbus-manager.c
+++ b/src/core/dbus-manager.c
@@ -1666,7 +1666,6 @@ static int install_error(
 static int method_enable_unit_files_generic(
                 sd_bus_message *message,
                 Manager *m,
-                const char *verb,
                 int (*call)(UnitFileScope scope, bool runtime, const char *root_dir, char *files[], bool force, UnitFileChange **changes, unsigned *n_changes),
                 bool carries_install_info,
                 sd_bus_error *error) {
@@ -1701,15 +1700,15 @@ static int method_enable_unit_files_generic(
 }
 
 static int method_enable_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) {
-        return method_enable_unit_files_generic(message, userdata, "enable", unit_file_enable, true, error);
+        return method_enable_unit_files_generic(message, userdata, unit_file_enable, true, error);
 }
 
 static int method_reenable_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) {
-        return method_enable_unit_files_generic(message, userdata, "enable", unit_file_reenable, true, error);
+        return method_enable_unit_files_generic(message, userdata, unit_file_reenable, true, error);
 }
 
 static int method_link_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) {
-        return method_enable_unit_files_generic(message, userdata, "enable", unit_file_link, false, error);
+        return method_enable_unit_files_generic(message, userdata, unit_file_link, false, error);
 }
 
 static int unit_file_preset_without_mode(UnitFileScope scope, bool runtime, const char *root_dir, char **files, bool force, UnitFileChange **changes, unsigned *n_changes) {
@@ -1717,11 +1716,11 @@ static int unit_file_preset_without_mode(UnitFileScope scope, bool runtime, cons
 }
 
 static int method_preset_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) {
-        return method_enable_unit_files_generic(message, userdata, "enable", unit_file_preset_without_mode, true, error);
+        return method_enable_unit_files_generic(message, userdata, unit_file_preset_without_mode, true, error);
 }
 
 static int method_mask_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) {
-        return method_enable_unit_files_generic(message, userdata, "disable", unit_file_mask, false, error);
+        return method_enable_unit_files_generic(message, userdata, unit_file_mask, false, error);
 }
 
 static int method_preset_unit_files_with_mode(sd_bus_message *message, void *userdata, sd_bus_error *error) {
@@ -1769,7 +1768,6 @@ static int method_preset_unit_files_with_mode(sd_bus_message *message, void *use
 static int method_disable_unit_files_generic(
                 sd_bus_message *message,
                 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) {
 
@@ -1803,11 +1801,11 @@ static int method_disable_unit_files_generic(
 }
 
 static int method_disable_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) {
-        return method_disable_unit_files_generic(message, userdata, "disable", unit_file_disable, error);
+        return method_disable_unit_files_generic(message, userdata, unit_file_disable, error);
 }
 
 static int method_unmask_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) {
-        return method_disable_unit_files_generic(message, userdata, "enable", unit_file_unmask, error);
+        return method_disable_unit_files_generic(message, userdata, unit_file_unmask, error);
 }
 
 static int method_revert_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) {
diff --git a/src/shared/install.c b/src/shared/install.c
index 71012eafb4..e97721b79e 100644
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -222,8 +222,8 @@ int unit_file_changes_add(
                 const char *path,
                 const char *source) {
 
+        _cleanup_free_ char *p = NULL, *s = NULL;
         UnitFileChange *c;
-        unsigned i;
 
         assert(path);
         assert(!changes == !n_changes);
@@ -234,29 +234,22 @@ int unit_file_changes_add(
         c = realloc(*changes, (*n_changes + 1) * sizeof(UnitFileChange));
         if (!c)
                 return -ENOMEM;
-
         *changes = c;
-        i = *n_changes;
 
-        c[i].type = type;
-        c[i].path = strdup(path);
-        if (!c[i].path)
-                return -ENOMEM;
+        p = strdup(path);
+        if (source)
+                s = strdup(source);
 
-        path_kill_slashes(c[i].path);
-
-        if (source) {
-                c[i].source = strdup(source);
-                if (!c[i].source) {
-                        free(c[i].path);
-                        return -ENOMEM;
-                }
+        if (!p || (source && !s))
+                return -ENOMEM;
 
-                path_kill_slashes(c[i].path);
-        } else
-                c[i].source = NULL;
+        path_kill_slashes(p);
+        if (s)
+                path_kill_slashes(s);
 
-        *n_changes = i+1;
+        c[*n_changes] = (UnitFileChange) { type, p, s };
+        p = s = NULL;
+        (*n_changes) ++;
         return 0;
 }
 
@@ -265,9 +258,6 @@ void unit_file_changes_free(UnitFileChange *changes, unsigned n_changes) {
 
         assert(changes || n_changes == 0);
 
-        if (!changes)
-                return;
-
         for (i = 0; i < n_changes; i++) {
                 free(changes[i].path);
                 free(changes[i].source);
@@ -370,8 +360,14 @@ static int create_symlink(
         }
 
         r = readlink_malloc(new_path, &dest);
-        if (r < 0)
+        if (r < 0) {
+                /* translate EINVAL (non-symlink exists) to EEXIST */
+                if (r == -EINVAL)
+                        r = -EEXIST;
+
+                unit_file_changes_add(changes, n_changes, r, new_path, NULL);
                 return r;
+        }
 
         if (path_equal(dest, old_path))
                 return 0;
@@ -382,8 +378,10 @@ static int create_symlink(
         }
 
         r = symlink_atomic(old_path, new_path);
-        if (r < 0)
+        if (r < 0) {
+                unit_file_changes_add(changes, n_changes, r, new_path, NULL);
                 return r;
+        }
 
         unit_file_changes_add(changes, n_changes, UNIT_FILE_UNLINK, new_path, NULL);
         unit_file_changes_add(changes, n_changes, UNIT_FILE_SYMLINK, new_path, old_path);
@@ -521,8 +519,8 @@ static int remove_marked_symlinks_fd(
 
                         unit_file_changes_add(changes, n_changes, UNIT_FILE_UNLINK, p, NULL);
 
-                        /* Now, remember the full path (but with the root prefix removed) of the symlink we just
-                         * removed, and remove any symlinks to it, too */
+                        /* Now, remember the full path (but with the root prefix removed) of
+                         * the symlink we just removed, and remove any symlinks to it, too. */
 
                         rp = skip_root(lp, p);
                         q = mark_symlink_for_removal(&remove_symlinks_to, rp ?: p);
@@ -1392,7 +1390,6 @@ static int install_info_symlink_wants(
                 const char *config_path,
                 char **list,
                 const char *suffix,
-                bool force,
                 UnitFileChange **changes,
                 unsigned *n_changes) {
 
@@ -1440,7 +1437,7 @@ static int install_info_symlink_wants(
 
                 rp = skip_root(paths, i->path);
 
-                q = create_symlink(rp ?: i->path, path, force, changes, n_changes);
+                q = create_symlink(rp ?: i->path, path, true, changes, n_changes);
                 if (r == 0)
                         r = q;
         }
@@ -1499,11 +1496,11 @@ static int install_info_apply(
 
         r = install_info_symlink_alias(i, paths, config_path, force, changes, n_changes);
 
-        q = install_info_symlink_wants(i, paths, config_path, i->wanted_by, ".wants/", force, changes, n_changes);
+        q = install_info_symlink_wants(i, paths, config_path, i->wanted_by, ".wants/", changes, n_changes);
         if (r == 0)
                 r = q;
 
-        q = install_info_symlink_wants(i, paths, config_path, i->required_by, ".requires/", force, changes, n_changes);
+        q = install_info_symlink_wants(i, paths, config_path, i->required_by, ".requires/", changes, n_changes);
         if (r == 0)
                 r = q;
 
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 059e985463..04205411dd 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -5390,6 +5390,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) {
         UnitFileChange *changes = NULL;
         unsigned n_changes = 0;
         int carries_install_info = -1;
+        bool ignore_carries_install_info = false;
         int r;
 
         if (!argv[1])
@@ -5420,7 +5421,6 @@ static int enable_unit(int argc, char *argv[], void *userdata) {
                         r = unit_file_link(arg_scope, arg_runtime, arg_root, names, arg_force, &changes, &n_changes);
                 else if (streq(verb, "preset")) {
                         r = unit_file_preset(arg_scope, arg_runtime, arg_root, names, arg_preset_mode, arg_force, &changes, &n_changes);
-                        carries_install_info = r;
                 } else if (streq(verb, "mask"))
                         r = unit_file_mask(arg_scope, arg_runtime, arg_root, names, arg_force, &changes, &n_changes);
                 else if (streq(verb, "unmask"))
@@ -5437,7 +5437,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) {
         } 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;
-                int expect_carries_install_info = false;
+                bool expect_carries_install_info = false;
                 bool send_runtime = true, send_force = true, send_preset_mode = false;
                 const char *method;
                 sd_bus *bus;
@@ -5468,6 +5468,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) {
                                 method = "PresetUnitFiles";
 
                         expect_carries_install_info = true;
+                        ignore_carries_install_info = true;
                 } else if (streq(verb, "mask"))
                         method = "MaskUnitFiles";
                 else if (streq(verb, "unmask")) {
@@ -5532,7 +5533,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) {
                         r = 0;
         }
 
-        if (carries_install_info == 0)
+        if (carries_install_info == 0 && !ignore_carries_install_info)
                 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"