From e8630e695232bdfcd16b55f3faafb4329c961104 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 18 Jul 2019 13:11:28 +0200 Subject: [PATCH] pid1: use a cache for all unit aliases This reworks how we load units from disk. Instead of chasing symlinks every time we are asked to load a unit by name, we slurp all symlinks from disk and build two hashmaps: 1. from unit name to either alias target, or fragment on disk (if an alias, we put just the target name in the hashmap, if a fragment we put an absolute path, so we can distinguish both). 2. from a unit name to all aliases Reading all this data can be pretty costly (40 ms) on my machine, so we keep it around for reuse. The advantage is that we can reliably know what all the aliases of a given unit are. This means we can reliably load dropins under all names. This fixes #11972. --- src/core/load-fragment.c | 350 ++++++---------------------- src/core/manager.c | 73 ++---- src/core/manager.h | 2 + src/core/unit.c | 3 + src/shared/unit-file.c | 353 +++++++++++++++++++++++++++++ src/shared/unit-file.h | 15 ++ src/systemctl/systemctl.c | 47 ++-- src/test/test-unit-file.c | 22 ++ test/TEST-15-DROPIN/test-dropin.sh | 33 ++- 9 files changed, 520 insertions(+), 378 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 3288b0b8388..7521d599e9e 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -4559,251 +4559,48 @@ int config_parse_ip_filter_bpf_progs( return 0; } -#define FOLLOW_MAX 8 - -static int open_follow(char **filename, FILE **_f, Set *names, char **_final) { - char *id = NULL; - unsigned c = 0; - int fd, r; - FILE *f; - - assert(filename); - assert(*filename); - assert(_f); - assert(names); - - /* This will update the filename pointer if the loaded file is - * reached by a symlink. The old string will be freed. */ - - for (;;) { - char *target, *name; - - if (c++ >= FOLLOW_MAX) - return -ELOOP; - - path_simplify(*filename, false); - - /* Add the file name we are currently looking at to - * the names of this unit, but only if it is a valid - * unit name. */ - name = basename(*filename); - if (unit_name_is_valid(name, UNIT_NAME_ANY)) { - - id = set_get(names, name); - if (!id) { - id = strdup(name); - if (!id) - return -ENOMEM; - - r = set_consume(names, id); - if (r < 0) - return r; - } - } - - /* Try to open the file name, but don't if its a symlink */ - fd = open(*filename, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); - if (fd >= 0) - break; - - if (errno != ELOOP) - return -errno; - - /* Hmm, so this is a symlink. Let's read the name, and follow it manually */ - r = readlink_and_make_absolute(*filename, &target); - if (r < 0) - return r; - - free_and_replace(*filename, target); - } - - f = fdopen(fd, "r"); - if (!f) { - safe_close(fd); - return -errno; - } - - *_f = f; - *_final = id; - - return 0; -} - static int merge_by_names(Unit **u, Set *names, const char *id) { char *k; int r; assert(u); assert(*u); - assert(names); - /* Let's try to add in all symlink names we found */ + /* Let's try to add in all names that are aliases of this unit */ while ((k = set_steal_first(names))) { + _cleanup_free_ _unused_ char *free_k = k; - /* First try to merge in the other name into our - * unit */ + /* First try to merge in the other name into our unit */ r = unit_merge_by_name(*u, k); if (r < 0) { Unit *other; - /* Hmm, we couldn't merge the other unit into - * ours? Then let's try it the other way - * round */ - - /* If the symlink name we are looking at is unit template, then - we must search for instance of this template */ - if (unit_name_is_valid(k, UNIT_NAME_TEMPLATE) && (*u)->instance) { - _cleanup_free_ char *instance = NULL; + /* Hmm, we couldn't merge the other unit into ours? Then let's try it the other way + * round. */ - r = unit_name_replace_instance(k, (*u)->instance, &instance); - if (r < 0) - return r; - - other = manager_get_unit((*u)->manager, instance); - } else - other = manager_get_unit((*u)->manager, k); + other = manager_get_unit((*u)->manager, k); + if (!other) + return r; /* return previous failure */ - free(k); - - if (other) { - r = unit_merge(other, *u); - if (r >= 0) { - *u = other; - return merge_by_names(u, names, NULL); - } - } - - return r; - } - - if (id == k) - unit_choose_id(*u, id); - - free(k); - } - - return 0; -} - -static int load_from_path(Unit *u, const char *path) { - _cleanup_set_free_free_ Set *symlink_names = NULL; - _cleanup_fclose_ FILE *f = NULL; - _cleanup_free_ char *filename = NULL; - char *id = NULL; - Unit *merged; - struct stat st; - int r; - - assert(u); - assert(path); - - symlink_names = set_new(&string_hash_ops); - if (!symlink_names) - return -ENOMEM; - - if (path_is_absolute(path)) { - - filename = strdup(path); - if (!filename) - return -ENOMEM; - - r = open_follow(&filename, &f, symlink_names, &id); - if (r < 0) { - filename = mfree(filename); - if (r != -ENOENT) - return r; - } - - } else { - char **p; - - STRV_FOREACH(p, u->manager->lookup_paths.search_path) { - - /* Instead of opening the path right away, we manually - * follow all symlinks and add their name to our unit - * name set while doing so */ - filename = path_make_absolute(path, *p); - if (!filename) - return -ENOMEM; - - if (u->manager->unit_path_cache && - !set_get(u->manager->unit_path_cache, filename)) - r = -ENOENT; - else - r = open_follow(&filename, &f, symlink_names, &id); - if (r >= 0) - break; - - /* ENOENT means that the file is missing or is a dangling symlink. - * ENOTDIR means that one of paths we expect to be is a directory - * is not a directory, we should just ignore that. - * EACCES means that the directory or file permissions are wrong. - */ - if (r == -EACCES) - log_debug_errno(r, "Cannot access \"%s\": %m", filename); - else if (!IN_SET(r, -ENOENT, -ENOTDIR)) + r = unit_merge(other, *u); + if (r < 0) return r; - filename = mfree(filename); - /* Empty the symlink names for the next run */ - set_clear_free(symlink_names); + *u = other; + return merge_by_names(u, names, NULL); } - } - - if (!filename) - /* Hmm, no suitable file found? */ - return 0; - - if (!unit_type_may_alias(u->type) && set_size(symlink_names) > 1) { - log_unit_warning(u, "Unit type of %s does not support alias names, refusing loading via symlink.", u->id); - return -ELOOP; - } - - merged = u; - r = merge_by_names(&merged, symlink_names, id); - if (r < 0) - return r; - - if (merged != u) { - u->load_state = UNIT_MERGED; - return 0; - } - - if (fstat(fileno(f), &st) < 0) - return -errno; - if (null_or_empty(&st)) { - u->load_state = UNIT_MASKED; - u->fragment_mtime = 0; - } else { - u->load_state = UNIT_LOADED; - u->fragment_mtime = timespec_load(&st.st_mtim); - - /* Now, parse the file contents */ - r = config_parse(u->id, filename, f, - UNIT_VTABLE(u)->sections, - config_item_perf_lookup, load_fragment_gperf_lookup, - CONFIG_PARSE_ALLOW_INCLUDE, u); - if (r < 0) - return r; - } - - free_and_replace(u->fragment_path, filename); - - if (u->source_path) { - if (stat(u->source_path, &st) >= 0) - u->source_mtime = timespec_load(&st.st_mtim); - else - u->source_mtime = 0; + if (streq_ptr(id, k)) + unit_choose_id(*u, id); } return 0; } int unit_load_fragment(Unit *u) { + const char *fragment; + _cleanup_set_free_free_ Set *names = NULL; int r; - Iterator i; - const char *t; assert(u); assert(u->load_state == UNIT_STUB); @@ -4814,79 +4611,80 @@ int unit_load_fragment(Unit *u) { return 0; } - /* First, try to find the unit under its id. We always look - * for unit files in the default directories, to make it easy - * to override things by placing things in /etc/systemd/system */ - r = load_from_path(u, u->id); - if (r < 0) + r = unit_file_find_fragment(u->manager->unit_id_map, + u->manager->unit_name_map, + u->id, + &fragment, + &names); + if (r < 0 && r != -ENOENT) return r; - /* Try to find an alias we can load this with */ - if (u->load_state == UNIT_STUB) { - SET_FOREACH(t, u->names, i) { - - if (t == u->id) - continue; - - r = load_from_path(u, t); - if (r < 0) - return r; - - if (u->load_state != UNIT_STUB) - break; - } - } - - /* And now, try looking for it under the suggested (originally linked) path */ - if (u->load_state == UNIT_STUB && u->fragment_path) { - - r = load_from_path(u, u->fragment_path); - if (r < 0) - return r; + if (fragment) { + /* Open the file, check if this is a mask, otherwise read. */ + _cleanup_fclose_ FILE *f = NULL; + struct stat st; - if (u->load_state == UNIT_STUB) - /* Hmm, this didn't work? Then let's get rid - * of the fragment path stored for us, so that - * we don't point to an invalid location. */ - u->fragment_path = mfree(u->fragment_path); - } + /* Try to open the file name. A symlink is OK, for example for linked files or masks. We + * expect that all symlinks within the lookup paths have been already resolved, but we don't + * verify this here. */ + f = fopen(fragment, "re"); + if (!f) + return log_unit_notice_errno(u, errno, "Failed to open %s: %m", fragment); - /* Look for a template */ - if (u->load_state == UNIT_STUB && u->instance) { - _cleanup_free_ char *k = NULL; + if (fstat(fileno(f), &st) < 0) + return -errno; - r = unit_name_template(u->id, &k); + r = free_and_strdup(&u->fragment_path, fragment); if (r < 0) return r; - r = load_from_path(u, k); - if (r < 0) { + if (null_or_empty(&st)) { + u->load_state = UNIT_MASKED; + u->fragment_mtime = 0; + } else { + u->load_state = UNIT_LOADED; + u->fragment_mtime = timespec_load(&st.st_mtim); + + /* Now, parse the file contents */ + r = config_parse(u->id, fragment, f, + UNIT_VTABLE(u)->sections, + config_item_perf_lookup, load_fragment_gperf_lookup, + CONFIG_PARSE_ALLOW_INCLUDE, u); if (r == -ENOEXEC) - log_unit_notice(u, "Unit configuration has fatal error, unit will not be started."); - return r; + log_unit_notice_errno(u, r, "Unit configuration has fatal error, unit will not be started."); + if (r < 0) + return r; } + } - if (u->load_state == UNIT_STUB) { - SET_FOREACH(t, u->names, i) { - _cleanup_free_ char *z = NULL; - - if (t == u->id) - continue; - - r = unit_name_template(t, &z); - if (r < 0) - return r; + /* We do the merge dance here because for some unit types, the unit might have aliases which are not + * declared in the file system. In particular, this is true (and frequent) for device and swap units. + */ + Unit *merged; + const char *id = u->id; + _cleanup_free_ char *free_id = NULL; - r = load_from_path(u, z); - if (r < 0) - return r; + if (fragment) { + id = basename(fragment); + if (unit_name_is_valid(id, UNIT_NAME_TEMPLATE)) { + assert(u->instance); /* If we're not trying to use a template for non-instanced unit, + * this must be set. */ - if (u->load_state != UNIT_STUB) - break; - } + r = unit_name_replace_instance(id, u->instance, &free_id); + if (r < 0) + return log_debug_errno(r, "Failed to build id (%s + %s): %m", id, u->instance); + id = free_id; } } + merged = u; + r = merge_by_names(&merged, names, id); + if (r < 0) + return r; + + if (merged != u) + u->load_state = UNIT_MERGED; + return 0; } diff --git a/src/core/manager.c b/src/core/manager.c index c176309edf3..f8a7e7d64e4 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -674,6 +674,12 @@ static int manager_setup_prefix(Manager *m) { return 0; } +static void manager_free_unit_name_maps(Manager *m) { + m->unit_id_map = hashmap_free(m->unit_id_map); + m->unit_name_map = hashmap_free(m->unit_name_map); + m->unit_path_cache = set_free_free(m->unit_path_cache); +} + static int manager_setup_run_queue(Manager *m) { int r; @@ -1357,7 +1363,7 @@ Manager* manager_free(Manager *m) { strv_free(m->client_environment); hashmap_free(m->cgroup_unit); - set_free_free(m->unit_path_cache); + manager_free_unit_name_maps(m); free(m->switch_root); free(m->switch_root_init); @@ -1461,56 +1467,6 @@ static void manager_catchup(Manager *m) { } } -static void manager_build_unit_path_cache(Manager *m) { - char **i; - int r; - - assert(m); - - set_free_free(m->unit_path_cache); - - m->unit_path_cache = set_new(&path_hash_ops); - if (!m->unit_path_cache) { - r = -ENOMEM; - goto fail; - } - - /* This simply builds a list of files we know exist, so that - * we don't always have to go to disk */ - - STRV_FOREACH(i, m->lookup_paths.search_path) { - _cleanup_closedir_ DIR *d = NULL; - struct dirent *de; - - d = opendir(*i); - if (!d) { - if (errno != ENOENT) - log_warning_errno(errno, "Failed to open directory %s, ignoring: %m", *i); - continue; - } - - FOREACH_DIRENT(de, d, r = -errno; goto fail) { - char *p; - - p = path_join(*i, de->d_name); - if (!p) { - r = -ENOMEM; - goto fail; - } - - r = set_consume(m->unit_path_cache, p); - if (r < 0) - goto fail; - } - } - - return; - -fail: - log_warning_errno(r, "Failed to build unit path cache, proceeding without: %m"); - m->unit_path_cache = set_free_free(m->unit_path_cache); -} - static void manager_distribute_fds(Manager *m, FDSet *fds) { Iterator i; Unit *u; @@ -1670,7 +1626,10 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { if (r < 0) log_warning_errno(r, "Failed to reduce unit file paths, ignoring: %m"); - manager_build_unit_path_cache(m); + manager_free_unit_name_maps(m); + r = unit_file_build_name_map(&m->lookup_paths, &m->unit_id_map, &m->unit_name_map, &m->unit_path_cache); + if (r < 0) + return log_error_errno(r, "Failed to build name map: %m"); { /* This block is (optionally) done with the reloading counter bumped */ @@ -2888,8 +2847,9 @@ int manager_loop(Manager *m) { assert(m); assert(m->objective == MANAGER_OK); /* Ensure manager_startup() has been called */ - /* Release the path cache */ - m->unit_path_cache = set_free_free(m->unit_path_cache); + /* Release the path and unit name caches */ + manager_free_unit_name_maps(m); + // FIXME: once this happens, we cannot load any more units manager_check_finished(m); @@ -3566,7 +3526,10 @@ int manager_reload(Manager *m) { if (r < 0) log_warning_errno(r, "Failed to reduce unit file paths, ignoring: %m"); - manager_build_unit_path_cache(m); + manager_free_unit_name_maps(m); + r = unit_file_build_name_map(&m->lookup_paths, &m->unit_id_map, &m->unit_name_map, &m->unit_path_cache); + if (r < 0) + log_warning_errno(r, "Failed to build name map: %m"); /* First, enumerate what we can from kernel and suchlike */ manager_enumerate_perpetual(m); diff --git a/src/core/manager.h b/src/core/manager.h index 9879082fea7..3cb37b3bf49 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -221,6 +221,8 @@ struct Manager { UnitFileScope unit_file_scope; LookupPaths lookup_paths; + Hashmap *unit_id_map; + Hashmap *unit_name_map; Set *unit_path_cache; char **transient_environment; /* The environment, as determined from config files, kernel cmdline and environment generators */ diff --git a/src/core/unit.c b/src/core/unit.c index fa89bd4a4d4..7561f1e1a1b 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -949,6 +949,9 @@ int unit_merge_by_name(Unit *u, const char *name) { Unit *other; int r; + /* Either add name to u, or if a unit with name already exists, merge it with u. + * If name is a template, do the same for name@instance, where instance is u's instance. */ + assert(u); assert(name); diff --git a/src/shared/unit-file.c b/src/shared/unit-file.c index cde38c472b8..90fa949d247 100644 --- a/src/shared/unit-file.c +++ b/src/shared/unit-file.c @@ -1,7 +1,14 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ +#include "dirent-util.h" +#include "fd-util.h" +#include "fs-util.h" #include "macro.h" +#include "path-lookup.h" +#include "set.h" +#include "stat-util.h" #include "string-util.h" +#include "strv.h" #include "unit-file.h" bool unit_type_may_alias(UnitType type) { @@ -94,3 +101,349 @@ int unit_validate_alias_symlink_and_warn(const char *filename, const char *targe return 0; } + +#define FOLLOW_MAX 8 + +static int unit_ids_map_get( + Hashmap *unit_ids_map, + const char *unit_name, + const char **ret_fragment_path) { + + /* Resolve recursively until we hit an absolute path, i.e. a non-aliased unit. + * + * We distinguish the case where unit_name was not found in the hashmap at all, and the case where + * some symlink was broken. + * + * If a symlink target points to an instance name, then we also check for the template. */ + + const char *id = NULL; + int r; + + for (unsigned n = 0; n < FOLLOW_MAX; n++) { + const char *t = hashmap_get(unit_ids_map, id ?: unit_name); + if (!t) { + _cleanup_free_ char *template = NULL; + + if (!id) + return -ENOENT; + + r = unit_name_template(id, &template); + if (r == -EINVAL) + return -ENXIO; /* we failed to find the symlink target */ + if (r < 0) + return log_error_errno(r, "Failed to determine template name for %s: %m", id); + + t = hashmap_get(unit_ids_map, template); + if (!t) + return -ENXIO; + + /* We successfully switched from instanced name to a template, let's continue */ + } + + if (path_is_absolute(t)) { + if (ret_fragment_path) + *ret_fragment_path = t; + return 0; + } + + id = t; + } + + return -ELOOP; +} + +int unit_file_build_name_map( + const LookupPaths *lp, + Hashmap **ret_unit_ids_map, + Hashmap **ret_unit_names_map, + Set **ret_path_cache) { + + /* Build two mappings: any name → main unit (i.e. the end result of symlink resolution), unit name → + * all aliases (i.e. the entry for a given key is a a list of all names which point to this key). The + * key is included in the value iff we saw a file or symlink with that name. In other words, if we + * have a key, but it is not present in the value for itself, there was an alias pointing to it, but + * the unit itself is not loadable. + * + * At the same, build a cache of paths where to find units. + */ + + _cleanup_hashmap_free_ Hashmap *ids = NULL, *names = NULL; + _cleanup_set_free_free_ Set *paths = NULL; + char **dir; + int r; + + if (ret_path_cache) { + paths = set_new(&path_hash_ops); + if (!paths) + return log_oom(); + } + + STRV_FOREACH(dir, (char**) lp->search_path) { + struct dirent *de; + _cleanup_closedir_ DIR *d = NULL; + + d = opendir(*dir); + if (!d) { + if (errno != ENOENT) + log_warning_errno(errno, "Failed to open \"%s\", ignoring: %m", *dir); + continue; + } + + FOREACH_DIRENT(de, d, log_warning_errno(errno, "Failed to read \"%s\", ignoring: %m", *dir)) { + char *filename; + _cleanup_free_ char *_filename_free = NULL, *simplified = NULL; + const char *suffix, *dst = NULL; + + filename = path_join(*dir, de->d_name); + if (!filename) + return log_oom(); + + if (ret_path_cache) { + r = set_consume(paths, filename); + if (r < 0) + return log_oom(); + /* We will still use filename below. This is safe because we know the set + * holds a reference. */ + } else + _filename_free = filename; /* Make sure we free the filename. */ + + if (!unit_name_is_valid(de->d_name, UNIT_NAME_ANY)) + continue; + assert_se(suffix = strrchr(de->d_name, '.')); + + /* search_path is ordered by priority (highest first). If the name is already mapped + * to something (incl. itself), it means that we have already seen it, and we should + * ignore it here. */ + if (hashmap_contains(ids, de->d_name)) + continue; + + if (de->d_type == DT_LNK) { + /* We don't explicitly check for alias loops here. unit_ids_map_get() which + * limits the number of hops should be used to access the map. */ + + _cleanup_free_ char *target = NULL, *target_abs = NULL; + + r = readlinkat_malloc(dirfd(d), de->d_name, &target); + if (r < 0) { + log_warning_errno(r, "Failed to read symlink %s/%s, ignoring: %m", + *dir, de->d_name); + continue; + } + + if (!path_is_absolute(target)) { + target_abs = path_join(*dir, target); + if (!target_abs) + return log_oom(); + + free_and_replace(target, target_abs); + } + + /* Get rid of "." and ".." components in target path */ + r = chase_symlinks(target, lp->root_dir, CHASE_NOFOLLOW | CHASE_NONEXISTENT, &simplified); + if (r < 0) { + log_warning_errno(r, "Failed to resolve symlink %s pointing to %s, ignoring: %m", + filename, target); + continue; + } + + /* Check if the symlink goes outside of our search path. + * If yes, it's a linked unit file or mask, and we don't care about the target name. + * Let's just store the link destination directly. + * If not, let's verify that it's a good symlink. */ + char *tail = path_startswith_strv(simplified, lp->search_path); + if (tail) { + bool self_alias; + + dst = basename(simplified); + self_alias = streq(dst, de->d_name); + + if (is_path(tail)) + log_full(self_alias ? LOG_DEBUG : LOG_WARNING, + "Suspicious symlink %s→%s, treating as alias.", + filename, simplified); + + r = unit_validate_alias_symlink_and_warn(filename, simplified); + if (r < 0) + continue; + + if (self_alias) { + /* A self-alias that has no effect */ + log_debug("%s: self-alias: %s/%s → %s, ignoring.", + __func__, *dir, de->d_name, dst); + continue; + } + + log_debug("%s: alias: %s/%s → %s", __func__, *dir, de->d_name, dst); + } else { + dst = simplified; + + log_debug("%s: linked unit file: %s/%s → %s", __func__, *dir, de->d_name, dst); + } + + } else { + dst = filename; + log_debug("%s: normal unit file: %s", __func__, dst); + } + + r = hashmap_put_strdup(&ids, de->d_name, dst); + if (r < 0) + return log_warning_errno(r, "Failed to add entry to hashmap (%s→%s): %m", + de->d_name, dst); + } + } + + /* Let's also put the names in the reverse db. */ + Iterator it; + const char *dummy, *src; + HASHMAP_FOREACH_KEY(dummy, src, ids, it) { + const char *dst; + + r = unit_ids_map_get(ids, src, &dst); + if (r < 0) + continue; + + if (null_or_empty_path(dst) != 0) + continue; + + /* Do not treat instance symlinks that point to the template as aliases */ + if (unit_name_is_valid(basename(dst), UNIT_NAME_TEMPLATE) && + unit_name_is_valid(src, UNIT_NAME_INSTANCE)) + continue; + + r = string_strv_hashmap_put(&names, basename(dst), src); + if (r < 0) + return log_warning_errno(r, "Failed to add entry to hashmap (%s→%s): %m", + basename(dst), src); + } + + *ret_unit_ids_map = TAKE_PTR(ids); + *ret_unit_names_map = TAKE_PTR(names); + if (ret_path_cache) + *ret_path_cache = TAKE_PTR(paths); + + return 0; +} + +int unit_file_find_fragment( + Hashmap *unit_ids_map, + Hashmap *unit_name_map, + const char *unit_name, + const char **ret_fragment_path, + Set **ret_names) { + + const char *fragment = NULL; + _cleanup_free_ char *template = NULL, *instance = NULL; + _cleanup_set_free_free_ Set *names = NULL; + char **t, **nnn; + int r, name_type; + + /* Finds a fragment path, and returns the set of names: + * if we have …/foo.service and …/foo-alias.service→foo.service, + * and …/foo@.service and …/foo-alias@.service→foo@.service, + * and …/foo@inst.service, + * this should return: + * foo.service → …/foo.service, {foo.service, foo-alias.service}, + * foo-alias.service → …/foo.service, {foo.service, foo-alias.service}, + * foo@.service → …/foo@.service, {foo@.service, foo-alias@.service}, + * foo-alias@.service → …/foo@.service, {foo@.service, foo-alias@.service}, + * foo@bar.service → …/foo@.service, {foo@bar.service, foo-alias@bar.service}, + * foo-alias@bar.service → …/foo@.service, {foo@bar.service, foo-alias@bar.service}, + * foo-alias@inst.service → …/foo@inst.service, {foo@inst.service, foo-alias@inst.service}. + */ + + name_type = unit_name_to_instance(unit_name, &instance); + if (name_type < 0) + return name_type; + + names = set_new(&string_hash_ops); + if (!names) + return -ENOMEM; + + /* The unit always has its own name if it's not a template. */ + if (unit_name_is_valid(unit_name, UNIT_NAME_PLAIN | UNIT_NAME_INSTANCE)) { + r = set_put_strdup(names, unit_name); + if (r < 0) + return r; + } + + /* First try to load fragment under the original name */ + r = unit_ids_map_get(unit_ids_map, unit_name, &fragment); + if (r < 0 && !IN_SET(r, -ENOENT, -ENXIO)) + return log_debug_errno(r, "Cannot load unit %s: %m", unit_name); + + if (fragment) { + /* Add any aliases of the original name to the set of names */ + nnn = hashmap_get(unit_name_map, basename(fragment)); + STRV_FOREACH(t, nnn) { + if (instance && unit_name_is_valid(*t, UNIT_NAME_TEMPLATE)) { + char *inst; + + r = unit_name_replace_instance(*t, instance, &inst); + if (r < 0) + return log_debug_errno(r, "Cannot build instance name %s+%s: %m", *t, instance); + + if (!streq(unit_name, inst)) + log_debug("%s: %s has alias %s", __func__, unit_name, inst); + + log_info("%s: %s+%s → %s", __func__, *t, instance, inst); + r = set_consume(names, inst); + } else { + if (!streq(unit_name, *t)) + log_debug("%s: %s has alias %s", __func__, unit_name, *t); + + r = set_put_strdup(names, *t); + } + if (r < 0) + return r; + } + } + + if (!fragment && name_type == UNIT_NAME_INSTANCE) { + /* Look for a fragment under the template name */ + + r = unit_name_template(unit_name, &template); + if (r < 0) + return log_error_errno(r, "Failed to determine template name: %m"); + + r = unit_ids_map_get(unit_ids_map, template, &fragment); + if (r < 0 && !IN_SET(r, -ENOENT, -ENXIO)) + return log_debug_errno(r, "Cannot load template %s: %m", *t); + + if (fragment) { + /* Add any aliases of the original name to the set of names */ + nnn = hashmap_get(unit_name_map, basename(fragment)); + STRV_FOREACH(t, nnn) { + _cleanup_free_ char *inst = NULL; + const char *inst_fragment = NULL; + + r = unit_name_replace_instance(*t, instance, &inst); + if (r < 0) + return log_debug_errno(r, "Cannot build instance name %s+%s: %m", template, instance); + + /* Exclude any aliases that point in some other direction. */ + r = unit_ids_map_get(unit_ids_map, inst, &inst_fragment); + if (r < 0 && !IN_SET(r, -ENOENT, -ENXIO)) + return log_debug_errno(r, "Cannot find instance fragment %s: %m", inst); + + if (inst_fragment && + !streq(basename(inst_fragment), basename(fragment))) { + log_debug("Instance %s has fragment %s and is not an alias of %s.", + inst, inst_fragment, unit_name); + continue; + } + + if (!streq(unit_name, inst)) + log_info("%s: %s has alias %s", __func__, unit_name, inst); + r = set_consume(names, TAKE_PTR(inst)); + if (r < 0) + return r; + } + } + } + + *ret_fragment_path = fragment; + *ret_names = TAKE_PTR(names); + + // FIXME: if instance, consider any unit names with different template name + return 0; +} diff --git a/src/shared/unit-file.h b/src/shared/unit-file.h index e57f472f4f7..52e17f7cfb1 100644 --- a/src/shared/unit-file.h +++ b/src/shared/unit-file.h @@ -3,10 +3,12 @@ #include +#include "hashmap.h" #include "unit-name.h" typedef enum UnitFileState UnitFileState; typedef enum UnitFileScope UnitFileScope; +typedef struct LookupPaths LookupPaths; enum UnitFileState { UNIT_FILE_ENABLED, @@ -37,3 +39,16 @@ bool unit_type_may_alias(UnitType type) _const_; bool unit_type_may_template(UnitType type) _const_; int unit_validate_alias_symlink_and_warn(const char *filename, const char *target); + +int unit_file_build_name_map( + const LookupPaths *lp, + Hashmap **ret_unit_ids_map, + Hashmap **ret_unit_names_map, + Set **ret_path_cache); + +int unit_file_find_fragment( + Hashmap *unit_ids_map, + Hashmap *unit_name_map, + const char *unit_name, + const char **ret_fragment_path, + Set **names); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 97f3176cc55..31d364cefed 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -33,6 +33,7 @@ #include "cgroup-util.h" #include "copy.h" #include "cpu-set-util.h" +#include "dirent-util.h" #include "dropin.h" #include "efivars.h" #include "env-util.h" @@ -165,12 +166,18 @@ static bool arg_jobs_before = false; static bool arg_jobs_after = false; static char **arg_clean_what = NULL; +/* This is a global cache that will be constructed on first use. */ +static Hashmap *cached_id_map = NULL; +static Hashmap *cached_name_map = NULL; + STATIC_DESTRUCTOR_REGISTER(arg_wall, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_root, freep); STATIC_DESTRUCTOR_REGISTER(arg_types, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_states, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_properties, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_clean_what, strv_freep); +STATIC_DESTRUCTOR_REGISTER(cached_id_map, hashmap_freep); +STATIC_DESTRUCTOR_REGISTER(cached_name_map, hashmap_freep); static int daemon_reload(int argc, char *argv[], void* userdata); static int trivial_method(int argc, char *argv[], void *userdata); @@ -2582,38 +2589,24 @@ static int unit_find_paths( return log_error_errno(r, "Failed to get DropInPaths: %s", bus_error_message(&error, r)); } } else { - _cleanup_set_free_ Set *names = NULL; - _cleanup_free_ char *template = NULL; + const char *_path; + _cleanup_set_free_free_ Set *names = NULL; - names = set_new(NULL); - if (!names) - return log_oom(); + if (!cached_name_map) { + r = unit_file_build_name_map(lp, &cached_id_map, &cached_name_map, NULL); + if (r < 0) + return r; + } - r = unit_find_template_path(unit_name, lp, &path, &template); + r = unit_file_find_fragment(cached_id_map, cached_name_map, unit_name, &_path, &names); if (r < 0) return r; - if (r > 0) { - if (null_or_empty_path(path)) - /* The template is masked. Let's cut the process short. */ - return -ERFKILL; - - /* We found the unit file. If we followed symlinks, this name might be - * different then the unit_name with started with. Look for dropins matching - * that "final" name. */ - r = set_put(names, basename(path)); - } else if (!template) - /* No unit file, let's look for dropins matching the original name. - * systemd has fairly complicated rules (based on unit type and provenience), - * which units are allowed not to have the main unit file. We err on the - * side of including too many files, and always try to load dropins. */ - r = set_put(names, unit_name); - else - /* The cases where we allow a unit to exist without the main file are - * never valid for templates. Don't try to load dropins in this case. */ - goto not_found; - if (r < 0) - return log_error_errno(r, "Failed to add unit name: %m"); + if (_path) { + path = strdup(_path); + if (!path) + return log_oom(); + } if (ret_dropin_paths) { r = unit_file_find_dropin_paths(arg_root, lp->search_path, NULL, diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index 5e281b28d5a..c5144a1b7ea 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -24,10 +24,32 @@ static void test_unit_validate_alias_symlink_and_warn(void) { assert_se(unit_validate_alias_symlink_and_warn("/path/a.slice", "/other/b.slice") == -EINVAL); } +static void test_unit_file_build_name_map(void) { + _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_hashmap_free_ Hashmap *unit_ids = NULL; + _cleanup_hashmap_free_ Hashmap *unit_names = NULL; + Iterator i; + const char *k, *dst; + char **v; + + assert_se(lookup_paths_init(&lp, UNIT_FILE_SYSTEM, 0, NULL) >= 0); + + assert_se(unit_file_build_name_map(&lp, &unit_ids, &unit_names, NULL) == 0); + + HASHMAP_FOREACH_KEY(dst, k, unit_ids, i) + log_info("ids: %s → %s", k, dst); + + HASHMAP_FOREACH_KEY(v, k, unit_names, i) { + _cleanup_free_ char *j = strv_join(v, ", "); + log_info("aliases: %s ← %s", k, j); + } +} + int main(int argc, char **argv) { test_setup_logging(LOG_DEBUG); test_unit_validate_alias_symlink_and_warn(); + test_unit_file_build_name_map(); return 0; } diff --git a/test/TEST-15-DROPIN/test-dropin.sh b/test/TEST-15-DROPIN/test-dropin.sh index f7856807c45..2cef5a3c5b5 100755 --- a/test/TEST-15-DROPIN/test-dropin.sh +++ b/test/TEST-15-DROPIN/test-dropin.sh @@ -158,14 +158,14 @@ EOF systemctl show -p Names,Requires bar@0 systemctl show -p Names,Requires bar-alias@0 check_ok bar@0 Names bar@0 - check_ko bar@0 Names bar-alias@0 + check_ok bar@0 Names bar-alias@0 check_ok bar@0 After bar-template-after.device check_ok bar@0 Requires bar-0-requires.device - check_ko bar@0 Requires bar-alias-0-requires.device + check_ok bar@0 Requires bar-alias-0-requires.device check_ok bar@0 Requires bar-template-requires.device - check_ko bar@0 Requires bar-alias-template-requires.device + check_ok bar@0 Requires bar-alias-template-requires.device check_ko bar@0 Requires yup-template-requires.device check_ok bar-alias@0 After bar-template-after.device @@ -181,15 +181,15 @@ EOF systemctl show -p Names,Requires bar@1 systemctl show -p Names,Requires bar-alias@1 check_ok bar@1 Names bar@1 - check_ko bar@1 Names bar-alias@1 + check_ok bar@1 Names bar-alias@1 check_ok bar@1 After bar-template-after.device check_ok bar@1 Requires bar-1-requires.device - check_ko bar@1 Requires bar-alias-1-requires.device + check_ok bar@1 Requires bar-alias-1-requires.device check_ok bar@1 Requires bar-template-requires.device # See https://github.com/systemd/systemd/pull/13119#discussion_r308145418 - check_ko bar@1 Requires bar-alias-template-requires.device + check_ok bar@1 Requires bar-alias-template-requires.device check_ko bar@1 Requires yup-template-requires.device check_ko bar@1 Requires yup-1-requires.device @@ -241,14 +241,14 @@ EOF check_ko bar@3 Requires yup-template-requires.device check_ko bar@3 Requires yup-3-requires.device - check_ok bar-alias@3 After bar-template-after.device + check_ko bar-alias@3 After bar-template-after.device - check_ok bar-alias@3 Requires bar-3-requires.device + check_ko bar-alias@3 Requires bar-3-requires.device check_ok bar-alias@3 Requires bar-alias-3-requires.device - check_ok bar-alias@3 Requires bar-template-requires.device + check_ko bar-alias@3 Requires bar-template-requires.device check_ok bar-alias@3 Requires bar-alias-template-requires.device - check_ko bar-alias@3 Requires yup-template-requires.device - check_ko bar-alias@3 Requires yup-3-requires.device + check_ok bar-alias@3 Requires yup-template-requires.device + check_ok bar-alias@3 Requires yup-3-requires.device clear_services foo {bar,yup,bar-alias}@{,1,2,3} } @@ -267,14 +267,7 @@ test_alias_dropins () { rm /etc/systemd/system/b1.service clear_services a b - # A weird behavior: the dependencies for 'a' may vary. It can be - # changed by loading an alias... - # - # [1] 'a1' is loaded and then "renamed" into 'a'. 'a1' is therefore - # part of the names set so all its specific dropins are loaded. - # - # [2] 'a' is already loaded. 'a1' is simply only merged into 'a' so - # none of its dropins are loaded ('y' is missing from the deps). + # Check that dependencies don't vary. echo "*** test 2" create_services a x y mkdir -p /etc/systemd/system/a1.service.wants/ @@ -285,7 +278,7 @@ test_alias_dropins () { check_ok a1 Wants y.service systemctl start a check_ok a1 Wants x.service # see [2] - check_ko a1 Wants y.service + check_ok a1 Wants y.service systemctl stop a x y rm /etc/systemd/system/a1.service -- 2.39.2