From 3f57bc2267e09c09c9f11cb0d104ee87267e452b Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 19 Dec 2019 16:53:12 +0100 Subject: [PATCH] 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. --- src/shared/install.c | 89 ++++++++++++--------- src/shared/install.h | 2 +- src/test/test-install-root.c | 148 +++++++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+), 37 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index df9f92bddc6..711638bd16b 100644 --- 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 12de2c66092..54d22a45d34 100644 --- 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 323e1124ba6..5b50f28118b 100644 --- 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; } -- 2.47.3