shared/install: rework alias check and add test

This mostly reuses existing checkers used by pid1, so handling of aliases
should be consistent. Hopefully, with the test it'll be clearer what it
happening.

Support for .wants/.requires "aliases" is restored. Those are still used in the
wild quite a bit, so we need to support them.

See https://github.com/systemd/systemd/pull/13119 for a discussion of aliases
with an instance that point to a different template: this is allowed.

diff --git a/src/shared/install.c b/src/shared/install.c
index df9f92bddc6685b7f24b7252ac26aaed5520bbb5..711638bd16b31125c17536aeb39034c2e9b18ed2 100644 (file)
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -1708,36 +1708,67 @@ static int install_info_discover_and_check(
         return install_info_may_process(ret ? *ret : NULL, paths, changes, n_changes);
 }
 
-int unit_file_verify_alias(const UnitFileInstallInfo *i, const char *dst) {
+int unit_file_verify_alias(const UnitFileInstallInfo *i, const char *dst, char **ret_dst) {
+        _cleanup_free_ char *dst_updated = NULL;
         int r;
 
-        if (unit_name_is_valid(i->name, UNIT_NAME_TEMPLATE) &&
-            !unit_name_is_valid(dst, UNIT_NAME_TEMPLATE) &&
-            !i->default_instance)
-                return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
-                                       "Aliases for templates without DefaultInstance must be templates.");
+        /* Verify that dst is a valid either a valid alias or a valid .wants/.requires symlink for the target
+         * unit *i. Return negative on error or if not compatible, zero on success.
+         *
+         * ret_dst is set in cases where "instance propagation" happens, i.e. when the instance part is
+         * inserted into dst. It is not normally set, even on success, so that the caller can easily
+         * distinguish the case where instance propagation occured.
+         */
+
+        const char *path_alias = strrchr(dst, '/');
+        if (path_alias) {
+                /* This branch covers legacy Alias= function of creating .wants and .requires symlinks. */
+                _cleanup_free_ char *dir = NULL;
 
-        if (unit_name_is_valid(i->name, UNIT_NAME_INSTANCE | UNIT_NAME_TEMPLATE) &&
-            !unit_name_is_valid(dst, UNIT_NAME_TEMPLATE | UNIT_NAME_INSTANCE))
-                return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
-                                       "Aliases for template instances must be a templates or template instances containing the instance name.");
+                path_alias ++; /* skip over slash */
 
-        if (unit_name_is_valid(i->name, UNIT_NAME_INSTANCE | UNIT_NAME_TEMPLATE) &&
-            unit_name_is_valid(dst, UNIT_NAME_INSTANCE)) {
-                _cleanup_free_ char *src_inst = NULL, *dst_inst = NULL;
+                dir = dirname_malloc(dst);
+                if (!dir)
+                        return log_oom();
 
-                r = unit_name_to_instance(i->name, &src_inst);
+                if (!endswith(dir, ".wants") && !endswith(dir, ".requires"))
+                        return log_warning_errno(SYNTHETIC_ERRNO(EXDEV),
+                                                 "Invalid path \"%s\" in alias.", dir);
+
+                /* That's the name we want to use for verification. */
+                r = unit_symlink_name_compatible(path_alias, i->name);
                 if (r < 0)
-                        return r;
+                        return log_error_errno(r, "Failed to verify alias validity: %m");
+                if (r == 0)
+                        return log_warning_errno(SYNTHETIC_ERRNO(EXDEV),
+                                                 "Invalid unit %s symlink %s.",
+                                                 i->name, dst);
+
+        } else {
+                /* If the symlink target has an instance set and the symlink source doesn't, we "propagate
+                 * the instance", i.e. instantiate the symlink source with the target instance. */
+                if (unit_name_is_valid(dst, UNIT_NAME_TEMPLATE)) {
+                        _cleanup_free_ char *inst = NULL;
+
+                        r = unit_name_to_instance(i->name, &inst);
+                        if (r < 0)
+                                return log_error_errno(r, "Failed to extract instance name from %s: %m", i->name);
+
+                        if (r == UNIT_NAME_INSTANCE) {
+                                r = unit_name_replace_instance(dst, inst, &dst_updated);
+                                if (r < 0)
+                                        return log_error_errno(r, "Failed to build unit name from %s+%s: %m",
+                                                               dst, inst);
+                        }
+                }
 
-                r = unit_name_to_instance(dst, &dst_inst);
+                r = unit_validate_alias_symlink_and_warn(dst_updated ?: dst, i->name);
                 if (r < 0)
                         return r;
-                if (!strstr(dst_inst, src_inst?src_inst:i->default_instance))
-                        return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
-                                               "Aliases for template instances must be a templates or template instances containing the instance name.");
+
         }
 
+        *ret_dst = TAKE_PTR(dst_updated);
         return 0;
 }
 
@@ -1757,31 +1788,17 @@ static int install_info_symlink_alias(
         assert(config_path);
 
         STRV_FOREACH(s, i->aliases) {
-                _cleanup_free_ char *alias_path = NULL, *dst = NULL;
+                _cleanup_free_ char *alias_path = NULL, *dst = NULL, *dst_updated = NULL;
 
                 q = install_full_printf(i, *s, &dst);
                 if (q < 0)
                         return q;
 
-                q = unit_file_verify_alias(i, dst);
+                q = unit_file_verify_alias(i, dst, &dst_updated);
                 if (q < 0)
                         continue;
 
-                if (unit_name_is_valid(i->name, UNIT_NAME_INSTANCE) &&
-                    unit_name_is_valid(dst, UNIT_NAME_TEMPLATE)) {
-
-                        _cleanup_free_ char *s_copy = NULL, *instance = NULL;
-                        q = unit_name_to_instance(i->name, &instance);
-                        if (q < 0)
-                                return q;
-
-                        q = unit_name_replace_instance(dst, instance, &s_copy);
-                        if (q < 0)
-                                return q;
-                        free_and_replace(dst, s_copy);
-                }
-
-                alias_path = path_make_absolute(dst, config_path);
+                alias_path = path_make_absolute(dst_updated ?: dst, config_path);
                 if (!alias_path)
                         return -ENOMEM;
 

diff --git a/src/shared/install.h b/src/shared/install.h
index 12de2c66092173e02da5bc94dfc057245d80a934..54d22a45d3476f428efdaa49eea41d674c117478 100644 (file)
--- a/src/shared/install.h
+++ b/src/shared/install.h
@@ -187,7 +187,7 @@ int unit_file_changes_add(UnitFileChange **changes, size_t *n_changes, UnitFileC
 void unit_file_changes_free(UnitFileChange *changes, size_t n_changes);
 void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *changes, size_t n_changes, bool quiet);
 
-int unit_file_verify_alias(const UnitFileInstallInfo *i, const char *dst);
+int unit_file_verify_alias(const UnitFileInstallInfo *i, const char *dst, char **ret_dst);
 
 int unit_file_query_preset(UnitFileScope scope, const char *root_dir, const char *name);
 

diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c
index 323e1124ba6f40167690aa607a3186f4d0559672..5b50f28118b4aefc955894ed2ff8f666ffbebb66 100644 (file)
--- a/src/test/test-install-root.c
+++ b/src/test/test-install-root.c
@@ -1043,6 +1043,152 @@ static void test_preset_multiple_instances(const char *root) {
         unit_file_changes_free(changes, n_changes);
 }
 
+static void verify_one(
+                const UnitFileInstallInfo *i,
+                const char *alias,
+                int expected,
+                const char *updated_name) {
+        int r;
+        static const UnitFileInstallInfo *last_info = NULL;
+        _cleanup_free_ char *alias2 = NULL;
+
+        if (i != last_info)
+                log_info("-- %s --", (last_info = i)->name);
+
+        r = unit_file_verify_alias(i, alias, &alias2);
+        log_info_errno(r, "alias %s ← %s: %d/%m (expected %d)%s%s%s",
+                       i->name, alias, r, expected,
+                       alias2 ? " [" : "", alias2 ?: "", alias2 ? "]" : "");
+        assert(r == expected);
+
+        /* This is is test for "instance propagation". This propagation matters mostly for WantedBy= and
+         * RequiredBy= settings, and less so for Alias=. The only case where it should happen is when we have
+         * an Alias=alias@.service an instantiated template template@instance. In that case the instance name
+         * should be propagated into the alias as alias@instance. */
+        assert(streq_ptr(alias2, updated_name));
+}
+
+static void test_verify_alias(void) {
+        const UnitFileInstallInfo
+                plain_service    = { .name = (char*) "plain.service" },
+                bare_template    = { .name = (char*) "template1@.service" },
+                di_template      = { .name = (char*) "template2@.service",
+                                     .default_instance = (char*) "di" },
+                inst_template    = { .name = (char*) "template3@inst.service" },
+                di_inst_template = { .name = (char*) "template4@inst.service",
+                                     .default_instance = (char*) "di" };
+
+        verify_one(&plain_service, "alias.service", 0, NULL);
+        verify_one(&plain_service, "alias.socket", -EXDEV, NULL);
+        verify_one(&plain_service, "alias@.service", -EXDEV, NULL);
+        verify_one(&plain_service, "alias@inst.service", -EXDEV, NULL);
+        verify_one(&plain_service, "foo.target.wants/plain.service", 0, NULL);
+        verify_one(&plain_service, "foo.target.wants/plain.socket", -EXDEV, NULL);
+        verify_one(&plain_service, "foo.target.wants/plain@.service", -EXDEV, NULL);
+        verify_one(&plain_service, "foo.target.wants/service", -EXDEV, NULL);
+        verify_one(&plain_service, "foo.target.requires/plain.service", 0, NULL);
+        verify_one(&plain_service, "foo.target.requires/plain.socket", -EXDEV, NULL);
+        verify_one(&plain_service, "foo.target.requires/plain@.service", -EXDEV, NULL);
+        verify_one(&plain_service, "foo.target.requires/service", -EXDEV, NULL);
+        verify_one(&plain_service, "foo.target.conf/plain.service", -EXDEV, NULL);
+
+        verify_one(&bare_template, "alias.service", -EXDEV, NULL);
+        verify_one(&bare_template, "alias.socket", -EXDEV, NULL);
+        verify_one(&bare_template, "alias@.socket", -EXDEV, NULL);
+        verify_one(&bare_template, "alias@inst.socket", -EXDEV, NULL);
+        /* A general alias alias@.service → template1@.service. */
+        verify_one(&bare_template, "alias@.service", 0, NULL);
+        /* Only a specific instance is aliased, see the discussion in https://github.com/systemd/systemd/pull/13119. */
+        verify_one(&bare_template, "alias@inst.service", 0, NULL);
+        verify_one(&bare_template, "foo.target.wants/plain.service", -EXDEV, NULL);
+        verify_one(&bare_template, "foo.target.wants/plain.socket", -EXDEV, NULL);
+        verify_one(&bare_template, "foo.target.wants/plain@.service", -EXDEV, NULL);
+         /* Name mistmatch: we cannot allow this, because plain@foo.service would be pulled in by foo.taget,
+          * but would not be resolvable on its own, since systemd doesn't know how to load the fragment. */
+        verify_one(&bare_template, "foo.target.wants/plain@foo.service", -EXDEV, NULL);
+        verify_one(&bare_template, "foo.target.wants/template1@foo.service", 0, NULL);
+        verify_one(&bare_template, "foo.target.wants/service", -EXDEV, NULL);
+        verify_one(&bare_template, "foo.target.requires/plain.service", -EXDEV, NULL);
+        verify_one(&bare_template, "foo.target.requires/plain.socket", -EXDEV, NULL);
+        verify_one(&bare_template, "foo.target.requires/plain@.service", -EXDEV, NULL); /* instance missing */
+        verify_one(&bare_template, "foo.target.requires/template1@inst.service", 0, NULL);
+        verify_one(&bare_template, "foo.target.requires/service", -EXDEV, NULL);
+        verify_one(&bare_template, "foo.target.conf/plain.service", -EXDEV, NULL);
+
+        verify_one(&di_template, "alias.service", -EXDEV, NULL);
+        verify_one(&di_template, "alias.socket", -EXDEV, NULL);
+        verify_one(&di_template, "alias@.socket", -EXDEV, NULL);
+        verify_one(&di_template, "alias@inst.socket", -EXDEV, NULL);
+        verify_one(&di_template, "alias@inst.service", 0, NULL);
+        verify_one(&di_template, "alias@.service", 0, NULL);
+        verify_one(&di_template, "alias@di.service", 0, NULL);
+        verify_one(&di_template, "foo.target.wants/plain.service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.wants/plain.socket", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.wants/plain@.service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.wants/plain@di.service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.wants/template2@di.service", 0, NULL);
+        verify_one(&di_template, "foo.target.wants/service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.requires/plain.service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.requires/plain.socket", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.requires/plain@.service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.requires/plain@di.service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.requires/plain@foo.service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.requires/template2@di.service", 0, NULL);
+        verify_one(&di_template, "foo.target.requires/service", -EXDEV, NULL);
+        verify_one(&di_template, "foo.target.conf/plain.service", -EXDEV, NULL);
+
+        verify_one(&inst_template, "alias.service", -EXDEV, NULL);
+        verify_one(&inst_template, "alias.socket", -EXDEV, NULL);
+        verify_one(&inst_template, "alias@.socket", -EXDEV, NULL);
+        verify_one(&inst_template, "alias@inst.socket", -EXDEV, NULL);
+        verify_one(&inst_template, "alias@inst.service", 0, NULL);
+        verify_one(&inst_template, "alias@.service", 0, "alias@inst.service");
+        verify_one(&inst_template, "alias@di.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.wants/plain.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.wants/plain.socket", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.wants/plain@.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.wants/plain@di.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.wants/plain@inst.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.wants/template3@foo.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.wants/template3@inst.service", 0, NULL);
+        verify_one(&inst_template, "bar.target.wants/service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.requires/plain.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.requires/plain.socket", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.requires/plain@.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.requires/plain@di.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.requires/plain@inst.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.requires/template3@foo.service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.requires/template3@inst.service", 0, NULL);
+        verify_one(&inst_template, "bar.target.requires/service", -EXDEV, NULL);
+        verify_one(&inst_template, "bar.target.conf/plain.service", -EXDEV, NULL);
+
+        /* explicit alias overrides DefaultInstance */
+        verify_one(&di_inst_template, "alias.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "alias.socket", -EXDEV, NULL);
+        verify_one(&di_inst_template, "alias@.socket", -EXDEV, NULL);
+        verify_one(&di_inst_template, "alias@inst.socket", -EXDEV, NULL);
+        verify_one(&di_inst_template, "alias@inst.service", 0, NULL);
+        verify_one(&di_inst_template, "alias@.service", 0, "alias@inst.service");
+        verify_one(&di_inst_template, "alias@di.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.wants/plain.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.wants/plain.socket", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.wants/plain@.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.wants/plain@di.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.wants/template4@foo.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.wants/template4@inst.service", 0, NULL);
+        verify_one(&di_inst_template, "goo.target.wants/template4@di.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.wants/service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.requires/plain.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.requires/plain.socket", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.requires/plain@.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.requires/plain@di.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.requires/plain@inst.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.requires/template4@foo.service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.requires/template4@inst.service", 0, NULL);
+        verify_one(&di_inst_template, "goo.target.requires/service", -EXDEV, NULL);
+        verify_one(&di_inst_template, "goo.target.conf/plain.service", -EXDEV, NULL);
+}
+
 int main(int argc, char *argv[]) {
         char root[] = "/tmp/rootXXXXXX";
         const char *p;
@@ -1080,5 +1226,7 @@ int main(int argc, char *argv[]) {
 
         assert_se(rm_rf(root, REMOVE_ROOT|REMOVE_PHYSICAL) >= 0);
 
+        test_verify_alias();
+
         return 0;
 }