From 3d0205f28b0663af16dc8ffc7d87b4f8541cf075 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Rosen?= Date: Tue, 3 Dec 2019 15:33:22 +0100 Subject: [PATCH 1/8] Be more strict about what can be an Alias for template and instances * Templates can only use other templates as alisases * Template instances can use templates or things that expand with an instance name --- src/shared/install.c | 55 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/shared/install.c b/src/shared/install.c index 0e673b3358..115a64f5f4 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1724,12 +1724,51 @@ static int install_info_symlink_alias( assert(config_path); STRV_FOREACH(s, i->aliases) { + _cleanup_free_ char *alias_path = NULL, *dst = NULL; q = install_full_printf(i, *s, &dst); if (q < 0) return q; + 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."); + } + + 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."); + } + + 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; + + q = unit_name_to_instance(i->name, &src_inst); + if (q < 0) + return q; + + q = unit_name_to_instance(dst, &dst_inst); + if (q < 0) + return q; + 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."); + } + + 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); if (!alias_path) return -ENOMEM; @@ -1737,6 +1776,22 @@ static int install_info_symlink_alias( q = create_symlink(paths, i->path, alias_path, force, changes, n_changes); if (r == 0) r = q; + + /* if we are a template with a DefaultInstance and we target a template, also create a link from the DefaultInstance*/ + if (unit_name_is_valid(i->name, UNIT_NAME_TEMPLATE) && unit_name_is_valid(dst, UNIT_NAME_TEMPLATE) && i->default_instance) { + _cleanup_free_ char *s_copy = NULL,*final_path = NULL; + + q = unit_name_replace_instance(dst,i->default_instance,&s_copy); + if (q < 0) + return q; + final_path = path_make_absolute(s_copy, config_path); + if (!final_path) + return -ENOMEM; + q = create_symlink(paths, i->path, final_path, force, changes, n_changes); + if (r == 0) + r = q; + } + } return r; From aa0f357fd833feecbea6c3e9be80b643e433bced Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 19 Dec 2019 15:04:17 +0100 Subject: [PATCH 2/8] shared/install: split out alias verification function No functional change. --- src/shared/install.c | 68 ++++++++++++++++++++++++++------------------ src/shared/install.h | 2 ++ 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 115a64f5f4..236f9a6e59 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1708,6 +1708,39 @@ 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 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."); + + 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."); + + 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; + + r = unit_name_to_instance(i->name, &src_inst); + if (r < 0) + return r; + + r = unit_name_to_instance(dst, &dst_inst); + 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."); + } + + return 0; +} + static int install_info_symlink_alias( UnitFileInstallInfo *i, const LookupPaths *paths, @@ -1724,46 +1757,25 @@ static int install_info_symlink_alias( assert(config_path); STRV_FOREACH(s, i->aliases) { - _cleanup_free_ char *alias_path = NULL, *dst = NULL; q = install_full_printf(i, *s, &dst); if (q < 0) return q; - 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."); - } + q = unit_file_verify_alias(i, dst); + if (q < 0) + continue; - 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."); - } + if (unit_name_is_valid(i->name, UNIT_NAME_INSTANCE) && + unit_name_is_valid(dst, UNIT_NAME_TEMPLATE)) { - 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; - - q = unit_name_to_instance(i->name, &src_inst); - if (q < 0) - return q; - - q = unit_name_to_instance(dst, &dst_inst); - if (q < 0) - return q; - 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."); - } - - 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); + q = unit_name_to_instance(i->name, &instance); if (q < 0) return q; - q = unit_name_replace_instance(dst,instance,&s_copy); + q = unit_name_replace_instance(dst, instance, &s_copy); if (q < 0) return q; free_and_replace(dst, s_copy); diff --git a/src/shared/install.h b/src/shared/install.h index b2ad9c4a71..12de2c6609 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -187,6 +187,8 @@ 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_query_preset(UnitFileScope scope, const char *root_dir, const char *name); const char *unit_file_state_to_string(UnitFileState s) _const_; From f9ef25a483ed976dd4248c5ddf1f3c052f3ebcba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 19 Dec 2019 17:13:48 +0100 Subject: [PATCH 3/8] basic/unit-name: make sure UnitNameFlags is signed Without that, a check like unit_name_to_instance(...) < 0 would not have the expected effect. --- src/basic/unit-name.h | 1 + src/test/test-unit-name.c | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/basic/unit-name.h b/src/basic/unit-name.h index 15ce4e2495..1cd33396d8 100644 --- a/src/basic/unit-name.h +++ b/src/basic/unit-name.h @@ -13,6 +13,7 @@ typedef enum UnitNameFlags { UNIT_NAME_TEMPLATE = 1 << 1, /* Allow foo@.service */ UNIT_NAME_INSTANCE = 1 << 2, /* Allow foo@bar.service */ UNIT_NAME_ANY = UNIT_NAME_PLAIN|UNIT_NAME_TEMPLATE|UNIT_NAME_INSTANCE, + _UNIT_NAME_INVALID = -1, } UnitNameFlags; bool unit_name_is_valid(const char *n, UnitNameFlags flags) _pure_; diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c index 5d18711a5e..6e294c72d6 100644 --- a/src/test/test-unit-name.c +++ b/src/test/test-unit-name.c @@ -78,6 +78,10 @@ static void test_unit_name_is_valid(void) { test_unit_name_is_valid_one("foo@%%i.service", UNIT_NAME_INSTANCE, false); test_unit_name_is_valid_one("foo@%%i%f.service", UNIT_NAME_INSTANCE, false); test_unit_name_is_valid_one("foo@%F.service", UNIT_NAME_INSTANCE, false); + + test_unit_name_is_valid_one("foo.target.wants/plain.service", UNIT_NAME_ANY, false); + test_unit_name_is_valid_one("foo.target.conf/foo.conf", UNIT_NAME_ANY, false); + test_unit_name_is_valid_one("foo.target.requires/plain.socket", UNIT_NAME_ANY, false); } static void test_unit_name_replace_instance_one(const char *pattern, const char *repl, const char *expected, int ret) { From b59817b199f3613c63cef36db63986b621dfc92a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 19 Dec 2019 16:55:11 +0100 Subject: [PATCH 4/8] shared/install: drop creation of alias for DefaultInstance It turns out that this is not necessary. When we try to resolve alias@inst, we first check alias@inst, and if that is not found, fall back to alias@. Since we already created a symlink for alias@, we will find that and the result will be the same. --- src/shared/install.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 236f9a6e59..df9f92bddc 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1788,22 +1788,6 @@ static int install_info_symlink_alias( q = create_symlink(paths, i->path, alias_path, force, changes, n_changes); if (r == 0) r = q; - - /* if we are a template with a DefaultInstance and we target a template, also create a link from the DefaultInstance*/ - if (unit_name_is_valid(i->name, UNIT_NAME_TEMPLATE) && unit_name_is_valid(dst, UNIT_NAME_TEMPLATE) && i->default_instance) { - _cleanup_free_ char *s_copy = NULL,*final_path = NULL; - - q = unit_name_replace_instance(dst,i->default_instance,&s_copy); - if (q < 0) - return q; - final_path = path_make_absolute(s_copy, config_path); - if (!final_path) - return -ENOMEM; - q = create_symlink(paths, i->path, final_path, force, changes, n_changes); - if (r == 0) - r = q; - } - } return r; From 9a4f9e69e10886a45549264b83b1c02c345c572f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 20 Dec 2019 19:12:34 +0100 Subject: [PATCH 5/8] shared/unit-file: expose function to check .wants/.requires symlink validity No functional change. --- src/core/load-dropin.c | 23 +++-------------------- src/shared/unit-file.c | 18 ++++++++++++++++++ src/shared/unit-file.h | 1 + 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index a50b200f5b..492e816e91 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -11,24 +11,6 @@ #include "unit-name.h" #include "unit.h" -static int unit_name_compatible(const char *a, const char *b) { - _cleanup_free_ char *template = NULL; - int r; - - /* The straightforward case: the symlink name matches the target */ - if (streq(a, b)) - return 1; - - r = unit_name_template(a, &template); - if (r == -EINVAL) - return 0; /* Not a template */ - if (r < 0) - return r; /* OOM, or some other failure. Just skip the warning. */ - - /* An instance name points to a target that is just the template name */ - return streq(template, b); -} - static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suffix) { _cleanup_strv_free_ char **paths = NULL; char **p; @@ -83,9 +65,10 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff /* We don't treat this as an error, especially because we didn't check this for a * long time. Nevertheless, we warn, because such mismatch can be mighty confusing. */ - r = unit_name_compatible(entry, basename(target)); + r = unit_symlink_name_compatible(entry, basename(target)); if (r < 0) { - log_unit_warning_errno(u, r, "Can't check if names %s and %s are compatible, ignoring: %m", entry, basename(target)); + log_unit_warning_errno(u, r, "Can't check if names %s and %s are compatible, ignoring: %m", + entry, basename(target)); continue; } if (r == 0) diff --git a/src/shared/unit-file.c b/src/shared/unit-file.c index 28cd3c8600..dc0ec78464 100644 --- a/src/shared/unit-file.c +++ b/src/shared/unit-file.c @@ -31,6 +31,24 @@ bool unit_type_may_template(UnitType type) { UNIT_PATH); } +int unit_symlink_name_compatible(const char *symlink, const char *target) { + _cleanup_free_ char *template = NULL; + int r; + + /* The straightforward case: the symlink name matches the target */ + if (streq(symlink, target)) + return 1; + + r = unit_name_template(symlink, &template); + if (r == -EINVAL) + return 0; /* Not a template */ + if (r < 0) + return r; + + /* An instance name points to a target that is just the template name */ + return streq(template, target); +} + int unit_validate_alias_symlink_and_warn(const char *filename, const char *target) { const char *src, *dst; _cleanup_free_ char *src_instance = NULL, *dst_instance = NULL; diff --git a/src/shared/unit-file.h b/src/shared/unit-file.h index 98ba677f3f..83f78618d3 100644 --- a/src/shared/unit-file.h +++ b/src/shared/unit-file.h @@ -39,6 +39,7 @@ enum UnitFileScope { bool unit_type_may_alias(UnitType type) _const_; bool unit_type_may_template(UnitType type) _const_; +int unit_symlink_name_compatible(const char *symlink, const char *target); int unit_validate_alias_symlink_and_warn(const char *filename, const char *target); int unit_file_build_name_map( From 3f57bc2267e09c09c9f11cb0d104ee87267e452b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 19 Dec 2019 16:53:12 +0100 Subject: [PATCH 6/8] 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 | 91 ++++++++++++--------- src/shared/install.h | 2 +- src/test/test-install-root.c | 148 +++++++++++++++++++++++++++++++++++ 3 files changed, 203 insertions(+), 38 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index df9f92bddc..711638bd16 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. + */ - 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."); + 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_INSTANCE)) { - _cleanup_free_ char *src_inst = NULL, *dst_inst = NULL; + path_alias ++; /* skip over slash */ - r = unit_name_to_instance(i->name, &src_inst); + dir = dirname_malloc(dst); + if (!dir) + return log_oom(); + + 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 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_validate_alias_symlink_and_warn(dst_updated ?: dst, i->name); if (r < 0) return r; - r = unit_name_to_instance(dst, &dst_inst); - 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 12de2c6609..54d22a45d3 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 323e1124ba..5b50f28118 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; } From 1bf15585521c4d23735f340215331cc333ec3a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 21 Dec 2019 16:09:35 +0100 Subject: [PATCH 7/8] core,install: allow one more case of "instance propagation" If we have a template unit template@.service, it should be allowed to specify a dependency on a unit without an instance, bar@.service. When the unit is created, the instance will be propagated into the target, so template@inst.service will depend on bar@inst.service. This commit changes unit_dependency_name_compatible(), which makes the manager accept links like that, and unit_file_verify_alias(), so that the installation function will agree to create a symlink like that, and finally the tests are adjusted to pass. --- src/core/load-dropin.c | 2 +- src/shared/install.c | 16 ++++++++++++++-- src/shared/unit-file.c | 27 ++++++++++++++++++++++----- src/shared/unit-file.h | 2 +- src/test/test-install-root.c | 16 ++++++++++++++++ 5 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index 492e816e91..f61e9da6f2 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -65,7 +65,7 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff /* We don't treat this as an error, especially because we didn't check this for a * long time. Nevertheless, we warn, because such mismatch can be mighty confusing. */ - r = unit_symlink_name_compatible(entry, basename(target)); + r = unit_symlink_name_compatible(entry, basename(target), u->instance); if (r < 0) { log_unit_warning_errno(u, r, "Can't check if names %s and %s are compatible, ignoring: %m", entry, basename(target)); diff --git a/src/shared/install.c b/src/shared/install.c index 711638bd16..2e3f2d565a 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1724,6 +1724,7 @@ int unit_file_verify_alias(const UnitFileInstallInfo *i, const char *dst, char * if (path_alias) { /* This branch covers legacy Alias= function of creating .wants and .requires symlinks. */ _cleanup_free_ char *dir = NULL; + char *p; path_alias ++; /* skip over slash */ @@ -1731,12 +1732,23 @@ int unit_file_verify_alias(const UnitFileInstallInfo *i, const char *dst, char * if (!dir) return log_oom(); - if (!endswith(dir, ".wants") && !endswith(dir, ".requires")) + p = endswith(dir, ".wants"); + if (!p) + p = endswith(dir, ".requires"); + if (!p) return log_warning_errno(SYNTHETIC_ERRNO(EXDEV), "Invalid path \"%s\" in alias.", dir); + *p = '\0'; /* dir should now be a unit name */ + + r = unit_name_classify(dir); + if (r < 0) + return log_warning_errno(SYNTHETIC_ERRNO(EXDEV), + "Invalid unit name component \"%s\" in alias.", dir); + + const bool instance_propagation = r == UNIT_NAME_TEMPLATE; /* That's the name we want to use for verification. */ - r = unit_symlink_name_compatible(path_alias, i->name); + r = unit_symlink_name_compatible(path_alias, i->name, instance_propagation); if (r < 0) return log_error_errno(r, "Failed to verify alias validity: %m"); if (r == 0) diff --git a/src/shared/unit-file.c b/src/shared/unit-file.c index dc0ec78464..7ad0ecc8fd 100644 --- a/src/shared/unit-file.c +++ b/src/shared/unit-file.c @@ -31,12 +31,15 @@ bool unit_type_may_template(UnitType type) { UNIT_PATH); } -int unit_symlink_name_compatible(const char *symlink, const char *target) { +int unit_symlink_name_compatible(const char *symlink, const char *target, bool instance_propagation) { _cleanup_free_ char *template = NULL; - int r; + int r, un_type1, un_type2; - /* The straightforward case: the symlink name matches the target */ - if (streq(symlink, target)) + un_type1 = unit_name_classify(symlink); + + /* The straightforward case: the symlink name matches the target and we have a valid unit */ + if (streq(symlink, target) && + (un_type1 & (UNIT_NAME_PLAIN | UNIT_NAME_INSTANCE))) return 1; r = unit_name_template(symlink, &template); @@ -45,8 +48,22 @@ int unit_symlink_name_compatible(const char *symlink, const char *target) { if (r < 0) return r; + un_type2 = unit_name_classify(target); + /* An instance name points to a target that is just the template name */ - return streq(template, target); + if (un_type1 == UNIT_NAME_INSTANCE && + un_type2 == UNIT_NAME_TEMPLATE && + streq(template, target)) + return 1; + + /* foo@.target.requires/bar@.service: instance will be propagated */ + if (instance_propagation && + un_type1 == UNIT_NAME_TEMPLATE && + un_type2 == UNIT_NAME_TEMPLATE && + streq(template, target)) + return 1; + + return 0; } int unit_validate_alias_symlink_and_warn(const char *filename, const char *target) { diff --git a/src/shared/unit-file.h b/src/shared/unit-file.h index 83f78618d3..a44ba5b051 100644 --- a/src/shared/unit-file.h +++ b/src/shared/unit-file.h @@ -39,7 +39,7 @@ enum UnitFileScope { bool unit_type_may_alias(UnitType type) _const_; bool unit_type_may_template(UnitType type) _const_; -int unit_symlink_name_compatible(const char *symlink, const char *target); +int unit_symlink_name_compatible(const char *symlink, const char *target, bool instance_propagation); int unit_validate_alias_symlink_and_warn(const char *filename, const char *target); int unit_file_build_name_map( diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index 5b50f28118..25498916f1 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -1091,6 +1091,8 @@ static void test_verify_alias(void) { 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(&plain_service, "foo.service/plain.service", -EXDEV, NULL); /* missing dir suffix */ + verify_one(&plain_service, "asdf.requires/plain.service", -EXDEV, NULL); /* invalid unit name component */ verify_one(&bare_template, "alias.service", -EXDEV, NULL); verify_one(&bare_template, "alias.socket", -EXDEV, NULL); @@ -1114,6 +1116,12 @@ static void test_verify_alias(void) { 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(&bare_template, "FOO@.target.requires/plain@.service", -EXDEV, NULL); /* template name mistatch */ + verify_one(&bare_template, "FOO@inst.target.requires/plain@.service", -EXDEV, NULL); + verify_one(&bare_template, "FOO@inst.target.requires/plain@inst.service", -EXDEV, NULL); + verify_one(&bare_template, "FOO@.target.requires/template1@.service", 0, NULL); /* instance propagated */ + verify_one(&bare_template, "FOO@inst.target.requires/template1@.service", -EXDEV, NULL); /* instance missing */ + verify_one(&bare_template, "FOO@inst.target.requires/template1@inst.service", 0, NULL); /* instance provided */ verify_one(&di_template, "alias.service", -EXDEV, NULL); verify_one(&di_template, "alias.socket", -EXDEV, NULL); @@ -1133,6 +1141,7 @@ static void test_verify_alias(void) { 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@.service", -EXDEV, NULL); /* instance missing */ 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); @@ -1161,6 +1170,13 @@ static void test_verify_alias(void) { 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); + verify_one(&inst_template, "BAR@.target.requires/plain@.service", -EXDEV, NULL); /* template name mistatch */ + verify_one(&inst_template, "BAR@inst.target.requires/plain@.service", -EXDEV, NULL); + verify_one(&inst_template, "BAR@inst.target.requires/plain@inst.service", -EXDEV, NULL); + verify_one(&inst_template, "BAR@.target.requires/template3@.service", -EXDEV, NULL); /* instance missing */ + verify_one(&inst_template, "BAR@inst.target.requires/template3@.service", -EXDEV, NULL); /* instance missing */ + verify_one(&inst_template, "BAR@inst.target.requires/template3@inst.service", 0, NULL); /* instance provided */ + verify_one(&inst_template, "BAR@inst.target.requires/template3@ins2.service", -EXDEV, NULL); /* instance mismatch */ /* explicit alias overrides DefaultInstance */ verify_one(&di_inst_template, "alias.service", -EXDEV, NULL); From 2e93770fd86576d1ad46d2cff7e11b2f4d90e3b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 9 Jan 2020 12:38:18 +0100 Subject: [PATCH 8/8] man: document alias rules and aliases dropin loading --- man/systemd.unit.xml | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 42508f3059..ca2030dc84 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -136,11 +136,25 @@ has the alias dbus-org.freedesktop.network1.service, created during installation as a symlink, so when systemd is asked through D-Bus to load dbus-org.freedesktop.network1.service, it'll load - systemd-networkd.service. Alias names may be used in commands like - disable, start, stop, status, - and similar, and in all unit dependency directives, including Wants=, - Requires=, Before=, After=. Aliases cannot be - used with the preset command. + systemd-networkd.service. As another example, default.target — + the default system target started at boot — is commonly symlinked (aliased) to either + multi-user.target or graphical.target to select what is started + by default. Alias names may be used in commands like disable, + start, stop, status, and similar, and in all + unit dependency directives, including Wants=, Requires=, + Before=, After=. Aliases cannot be used with the + preset command. + + Aliases obey the following restrictions: a unit of a certain type (.service, + .socket, …) can only be aliased by a name with the same type suffix. A plain unit (not + a template or an instance), may only be aliased by a plain name. A template instance may only be aliased + by another template instance, and the instance part must be identical. A template may be aliased by + another template (in which case the alias applies to all instances of the template). As a special case, a + template instance (e.g. alias@inst.service) may be a symlink to different template + (e.g. template@inst.service). In that case, just this specific instance is aliased, + while other instances of the template (e.g. alias@foo.service, + alias@bar.service) are not aliased. Those rule preserve the requirement that the + instance (if any) is always uniquely defined for a given unit and all its aliases. Unit files may specify aliases through the Alias= directive in the [Install] section. When the unit is enabled, symlinks will be created for those names, and removed when the unit is @@ -184,6 +198,16 @@ i.e. foo-bar-.service.d/10-override.conf overrides foo-.service.d/10-override.conf. + In cases of unit aliases (described above), dropins for the aliased name and all aliases are + loaded. In the example of default.target aliasing + graphical.target, default.target.d/, + default.target.wants/, default.target.requires/, + graphical.target.d/, graphical.target.wants/, + graphical.target.requires/ would all be read. For templates, dropins for the + template, any template aliases, the template instance, and all alias instances are read. When just a + specific template instance is aliased, then the dropins for the target template, the target template + instance, and the alias template instance are read. + In addition to /etc/systemd/system, the drop-in .d/ directories for system services can be placed in /usr/lib/systemd/system or /run/systemd/system directories. Drop-in files in /etc