Merge pull request #3055 from keszybz/preset-fixes

Another bunch of improvements to the installation code

3 files changed, 224 insertions, 55 deletions

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_;