From 9f6cbcf53cd3abe416b9808bbfb5648c6b808400 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 Feb 2017 16:22:49 +0100 Subject: install: don't enter loop when traversing a template symlinks Before this patch, if we'd encounter an instance or template symlink while traversing a chain of symlinks we'd fill in the instance name and retry the iteration. This makes no sense if the resulting name is actually the same as we are coming from, as we'd just spin a couple of times in the loop, until the UNIT_FILE_FOLLOW_SYMLINK_MAX iteration limit is hit. Fix this, by accepted the symlink as it is, if it identical to what we filled in. --- src/shared/install.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index 478abac8ab..975a789ebe 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1354,7 +1354,8 @@ static int install_info_follow( InstallContext *c, UnitFileInstallInfo *i, const char *root_dir, - SearchFlags flags) { + SearchFlags flags, + bool ignore_different_name) { assert(c); assert(i); @@ -1367,7 +1368,7 @@ static int install_info_follow( /* If the basename doesn't match, the caller should add a * complete new entry for this. */ - if (!streq(basename(i->symlink_target), i->name)) + if (!ignore_different_name && !streq(basename(i->symlink_target), i->name)) return -EXDEV; free_and_replace(i->path, i->symlink_target); @@ -1415,7 +1416,7 @@ static int install_info_traverse( return -ELOOP; } - r = install_info_follow(c, i, paths->root_dir, flags); + r = install_info_follow(c, i, paths->root_dir, flags, false); if (r == -EXDEV) { _cleanup_free_ char *buffer = NULL; const char *bn; @@ -1439,6 +1440,18 @@ static int install_info_traverse( if (r < 0) return r; + if (streq(buffer, i->name)) { + + /* We filled in the instance, and the target stayed the same? If so, then let's + * honour the link as it is. */ + + r = install_info_follow(c, i, paths->root_dir, flags, true); + if (r < 0) + return r; + + continue; + } + bn = buffer; } -- cgit v1.2.3-54-g00ecf From dfead90d936d4c4cbe4f164f35def7399d80817c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 Feb 2017 20:16:12 +0100 Subject: install: when a template unit is instantiated via a /usr symlink, consider it enabled If a unit foobar@.service stored below /usr is instantiated via a symlink foobar@quux.service also below /usr, then we should consider the instance statically enabled, while the template itself should continue to be considered enabled/disabled/static depending on its [Install] section. In order to implement this we'll now look for enablement symlinks in all unit search paths, not just in the config and runtime dirs. Fixes: #5136 --- src/shared/install.c | 119 ++++++++++++++++++++++++++++++------------- src/test/test-install-root.c | 23 +++++++++ 2 files changed, 107 insertions(+), 35 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index 975a789ebe..b938507587 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -208,7 +208,7 @@ static int path_is_control(const LookupPaths *p, const char *path) { path_equal_ptr(parent, p->runtime_control); } -static int path_is_config(const LookupPaths *p, const char *path) { +static int path_is_config(const LookupPaths *p, const char *path, bool check_parent) { _cleanup_free_ char *parent = NULL; assert(p); @@ -217,15 +217,19 @@ static int path_is_config(const LookupPaths *p, const char *path) { /* Note that we do *not* have generic checks for /etc or /run in place, since with * them we couldn't discern configuration from transient or generated units */ - parent = dirname_malloc(path); - if (!parent) - return -ENOMEM; + if (check_parent) { + parent = dirname_malloc(path); + if (!parent) + return -ENOMEM; - return path_equal_ptr(parent, p->persistent_config) || - path_equal_ptr(parent, p->runtime_config); + path = parent; + } + + return path_equal_ptr(path, p->persistent_config) || + path_equal_ptr(path, p->runtime_config); } -static int path_is_runtime(const LookupPaths *p, const char *path) { +static int path_is_runtime(const LookupPaths *p, const char *path, bool check_parent) { _cleanup_free_ char *parent = NULL; const char *rpath; @@ -239,16 +243,20 @@ static int path_is_runtime(const LookupPaths *p, const char *path) { if (rpath && path_startswith(rpath, "/run")) return true; - parent = dirname_malloc(path); - if (!parent) - return -ENOMEM; + if (check_parent) { + parent = dirname_malloc(path); + if (!parent) + return -ENOMEM; - return path_equal_ptr(parent, p->runtime_config) || - path_equal_ptr(parent, p->generator) || - path_equal_ptr(parent, p->generator_early) || - path_equal_ptr(parent, p->generator_late) || - path_equal_ptr(parent, p->transient) || - path_equal_ptr(parent, p->runtime_control); + path = parent; + } + + return path_equal_ptr(path, p->runtime_config) || + path_equal_ptr(path, p->generator) || + path_equal_ptr(path, p->generator_early) || + path_equal_ptr(path, p->generator_late) || + path_equal_ptr(path, p->transient) || + path_equal_ptr(path, p->runtime_control); } static int path_is_vendor(const LookupPaths *p, const char *path) { @@ -826,7 +834,9 @@ static int find_symlinks_in_scope( const char *name, UnitFileState *state) { - bool same_name_link_runtime = false, same_name_link = false; + bool same_name_link_runtime = false, same_name_link_config = false; + bool enabled_in_runtime = false, enabled_at_all = false; + char **p; int r; assert(scope >= 0); @@ -834,27 +844,66 @@ static int find_symlinks_in_scope( assert(paths); assert(name); - /* First look in the persistent config path */ - r = find_symlinks(paths->root_dir, name, paths->persistent_config, paths, &same_name_link); - if (r < 0) - return r; - if (r > 0) { - *state = UNIT_FILE_ENABLED; - return r; + STRV_FOREACH(p, paths->search_path) { + bool same_name_link = false; + + r = find_symlinks(paths->root_dir, name, *p, paths, &same_name_link); + if (r < 0) + return r; + if (r > 0) { + /* We found symlinks in this dir? Yay! Let's see where precisely it is enabled. */ + + r = path_is_config(paths, *p, false); + if (r < 0) + return r; + if (r > 0) { + /* This is the best outcome, let's return it immediately. */ + *state = UNIT_FILE_ENABLED; + return 1; + } + + r = path_is_runtime(paths, *p, false); + if (r < 0) + return r; + if (r > 0) + enabled_in_runtime = true; + else + enabled_at_all = true; + + } else if (same_name_link) { + + r = path_is_config(paths, *p, false); + if (r < 0) + return r; + if (r > 0) + same_name_link_config = true; + else { + r = path_is_runtime(paths, *p, false); + if (r < 0) + return r; + if (r > 0) + same_name_link_runtime = true; + } + } } - /* Then look in runtime config path */ - r = find_symlinks(paths->root_dir, name, paths->runtime_config, paths, &same_name_link_runtime); - if (r < 0) - return r; - if (r > 0) { + if (enabled_in_runtime) { *state = UNIT_FILE_ENABLED_RUNTIME; - return r; + return 1; + } + + /* Here's a special rule: if the unit we are looking for is an instance, and it symlinked in the search path + * outside of runtime and configuration directory, then we consider it statically enabled. Note we do that only + * for instance, not for regular names, as those are merely aliases, while instances explicitly instantiate + * something, and hence are a much stronger concept. */ + if (enabled_at_all && unit_name_is_valid(name, UNIT_NAME_INSTANCE)) { + *state = UNIT_FILE_STATIC; + return 1; } /* Hmm, we didn't find it, but maybe we found the same name * link? */ - if (same_name_link) { + if (same_name_link_config) { *state = UNIT_FILE_LINKED; return 1; } @@ -1409,7 +1458,7 @@ static int install_info_traverse( return -ELOOP; if (!(flags & SEARCH_FOLLOW_CONFIG_SYMLINKS)) { - r = path_is_config(paths, i->path); + r = path_is_config(paths, i->path, true); if (r < 0) return r; if (r > 0) @@ -2040,7 +2089,7 @@ static int path_shall_revert(const LookupPaths *paths, const char *path) { /* Checks whether the path is one where the drop-in directories shall be removed. */ - r = path_is_config(paths, path); + r = path_is_config(paths, path, true); if (r != 0) return r; @@ -2148,7 +2197,7 @@ int unit_file_revert( if (errno != ENOENT) return -errno; } else if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) { - r = path_is_config(&paths, path); + r = path_is_config(&paths, path, true); if (r < 0) return r; if (r > 0) { @@ -2494,7 +2543,7 @@ static int unit_file_lookup_state( switch (i->type) { case UNIT_FILE_TYPE_MASKED: - r = path_is_runtime(paths, i->path); + r = path_is_runtime(paths, i->path, true); if (r < 0) return r; diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index d0bc8004f3..575401cb91 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -736,6 +736,28 @@ static void test_preset_order(const char *root) { assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "prefix-2.service", &state) >= 0 && state == UNIT_FILE_DISABLED); } +static void test_static_instance(const char *root) { + UnitFileState state; + const char *p; + + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "static-instance@.service", &state) == -ENOENT); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "static-instance@foo.service", &state) == -ENOENT); + + p = strjoina(root, "/usr/lib/systemd/system/static-instance@.service"); + assert_se(write_string_file(p, + "[Install]\n" + "WantedBy=multi-user.target\n", WRITE_STRING_FILE_CREATE) >= 0); + + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "static-instance@.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "static-instance@foo.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + + p = strjoina(root, "/usr/lib/systemd/system/static-instance@foo.service"); + assert_se(symlink("static-instance@.service", p) >= 0); + + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "static-instance@.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "static-instance@foo.service", &state) >= 0 && state == UNIT_FILE_STATIC); +} + int main(int argc, char *argv[]) { char root[] = "/tmp/rootXXXXXX"; const char *p; @@ -766,6 +788,7 @@ int main(int argc, char *argv[]) { test_preset_and_list(root); test_preset_order(root); test_revert(root); + test_static_instance(root); assert_se(rm_rf(root, REMOVE_ROOT|REMOVE_PHYSICAL) >= 0); -- cgit v1.2.3-54-g00ecf From 80cb9da358de5601f5d23c8e5ed9a43eb3506f7d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 Feb 2017 20:22:09 +0100 Subject: install: remove some unused parameters from various functions in install.c No need to pass what we don't use. --- src/shared/install.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index b938507587..f25ed685f6 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -685,7 +685,6 @@ static int find_symlinks_fd( int fd, const char *path, const char *config_path, - const LookupPaths *lp, bool *same_name_link) { _cleanup_closedir_ DIR *d = NULL; @@ -696,7 +695,6 @@ static int find_symlinks_fd( assert(fd >= 0); assert(path); assert(config_path); - assert(lp); assert(same_name_link); d = fdopendir(fd); @@ -730,7 +728,7 @@ static int find_symlinks_fd( } /* This will close nfd, regardless whether it succeeds or not */ - q = find_symlinks_fd(root_dir, name, nfd, p, config_path, lp, same_name_link); + q = find_symlinks_fd(root_dir, name, nfd, p, config_path, same_name_link); if (q > 0) return 1; if (r == 0) @@ -808,7 +806,6 @@ static int find_symlinks( const char *root_dir, const char *name, const char *config_path, - const LookupPaths *lp, bool *same_name_link) { int fd; @@ -825,11 +822,10 @@ static int find_symlinks( } /* This takes possession of fd and closes it */ - return find_symlinks_fd(root_dir, name, fd, config_path, config_path, lp, same_name_link); + return find_symlinks_fd(root_dir, name, fd, config_path, config_path, same_name_link); } static int find_symlinks_in_scope( - UnitFileScope scope, const LookupPaths *paths, const char *name, UnitFileState *state) { @@ -839,15 +835,13 @@ static int find_symlinks_in_scope( char **p; int r; - assert(scope >= 0); - assert(scope < _UNIT_FILE_SCOPE_MAX); assert(paths); assert(name); STRV_FOREACH(p, paths->search_path) { bool same_name_link = false; - r = find_symlinks(paths->root_dir, name, *p, paths, &same_name_link); + r = find_symlinks(paths->root_dir, name, *p, &same_name_link); if (r < 0) return r; if (r > 0) { @@ -2567,7 +2561,7 @@ static int unit_file_lookup_state( break; } - r = find_symlinks_in_scope(scope, paths, i->name, &state); + r = find_symlinks_in_scope(paths, i->name, &state); if (r < 0) return r; if (r == 0) { -- cgit v1.2.3-54-g00ecf