packages/systemd
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
systemd/revert-pid1-use-a-cache-for-all-unit-aliases.patch
openeuler-basic bf6226aeee solve the memleak of pid1 and add some patches
2020-04-27 17:46:10 +08:00

1227 lines
49 KiB
Diff
Raw Blame History

From e8630e695232bdfcd16b55f3faafb4329c961104 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
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.
revert pid1 use a cache for all unit aliases
---
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 | 362 -------------------------------------
src/shared/unit-file.h | 15 --
src/systemctl/systemctl.c | 47 +++--
src/test/test-unit-file.c | 40 ----
test/TEST-15-DROPIN/test-dropin.sh | 33 ++--
9 files changed, 378 insertions(+), 547 deletions(-)
diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index 9821a92..9dd86e8 100644
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -4553,48 +4553,251 @@ 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 names that are aliases of this unit */
+ /* Let's try to add in all symlink names we found */
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. */
+ /* Hmm, we couldn't merge the other unit into
+ * ours? Then let's try it the other way
+ * round */
- other = manager_get_unit((*u)->manager, k);
- if (!other)
- return r; /* return previous failure */
+ /* 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;
- r = unit_merge(other, *u);
- if (r < 0)
- return r;
+ r = unit_name_replace_instance(k, (*u)->instance, &instance);
+ if (r < 0)
+ return r;
- *u = other;
- return merge_by_names(u, names, NULL);
+ other = manager_get_unit((*u)->manager, instance);
+ } else
+ other = manager_get_unit((*u)->manager, k);
+
+ free(k);
+
+ if (other) {
+ r = unit_merge(other, *u);
+ if (r >= 0) {
+ *u = other;
+ return merge_by_names(u, names, NULL);
+ }
+ }
+
+ return r;
}
- if (streq_ptr(id, k))
+ 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))
+ return r;
+
+ filename = mfree(filename);
+ /* Empty the symlink names for the next run */
+ set_clear_free(symlink_names);
+ }
+ }
+
+ 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;
}
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);
@@ -4605,79 +4808,78 @@ int unit_load_fragment(Unit *u) {
return 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)
+ /* 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)
return r;
- if (fragment) {
- /* Open the file, check if this is a mask, otherwise read. */
- _cleanup_fclose_ FILE *f = NULL;
- struct stat st;
+ /* Try to find an alias we can load this with */
+ if (u->load_state == UNIT_STUB) {
+ SET_FOREACH(t, u->names, i) {
- /* 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);
+ if (t == u->id)
+ continue;
- if (fstat(fileno(f), &st) < 0)
- return -errno;
+ r = load_from_path(u, t);
+ if (r < 0)
+ return r;
- r = free_and_strdup(&u->fragment_path, fragment);
+ 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 (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_errno(u, r, "Unit configuration has fatal error, unit will not be started.");
- if (r < 0)
- return r;
- }
+ 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);
}
- /* 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;
+ /* Look for a template */
+ if (u->load_state == UNIT_STUB && u->instance) {
+ _cleanup_free_ char *k = NULL;
- 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. */
+ r = unit_name_template(u->id, &k);
+ if (r < 0)
+ return r;
- 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;
+ r = load_from_path(u, k);
+ if (r < 0) {
+ if (r == -ENOEXEC)
+ log_unit_notice(u, "Unit configuration has fatal error, unit will not be started.");
+ return r;
}
- }
- merged = u;
- r = merge_by_names(&merged, names, id);
- if (r < 0)
- return r;
+ if (u->load_state == UNIT_STUB) {
+ SET_FOREACH(t, u->names, i) {
+ _cleanup_free_ char *z = NULL;
- if (merged != u)
- u->load_state = UNIT_MERGED;
+ if (t == u->id)
+ continue;
+
+ r = unit_name_template(t, &z);
+ if (r < 0)
+ return r;
+
+ r = load_from_path(u, z);
+ if (r < 0)
+ return r;
+
+ if (u->load_state != UNIT_STUB)
+ break;
+ }
+ }
+ }
return 0;
}
diff --git a/src/core/manager.c b/src/core/manager.c
index 8b1ce70..cfeaca6 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -689,12 +689,6 @@ 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;
@@ -1383,7 +1377,7 @@ Manager* manager_free(Manager *m) {
strv_free(m->client_environment);
hashmap_free(m->cgroup_unit);
- manager_free_unit_name_maps(m);
+ set_free_free(m->unit_path_cache);
free(m->switch_root);
free(m->switch_root_init);
@@ -1487,6 +1481,56 @@ 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;
@@ -1641,10 +1685,7 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) {
lookup_paths_log(&m->lookup_paths);
- 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");
+ manager_build_unit_path_cache(m);
{
/* This block is (optionally) done with the reloading counter bumped */
@@ -2862,9 +2903,8 @@ int manager_loop(Manager *m) {
assert(m);
assert(m->objective == MANAGER_OK); /* Ensure manager_startup() has been called */
- /* Release the path and unit name caches */
- manager_free_unit_name_maps(m);
- // FIXME: once this happens, we cannot load any more units
+ /* Release the path cache */
+ m->unit_path_cache = set_free_free(m->unit_path_cache);
manager_check_finished(m);
@@ -3539,10 +3579,7 @@ int manager_reload(Manager *m) {
lookup_paths_log(&m->lookup_paths);
- 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");
+ manager_build_unit_path_cache(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 9ca82ac..daeb454 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -220,8 +220,6 @@ 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 f96e6e4..1b8d5d9 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -959,9 +959,6 @@ 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 bad92a3..cde38c4 100644
--- a/src/shared/unit-file.c
+++ b/src/shared/unit-file.c
@@ -1,14 +1,7 @@
/* 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) {
@@ -101,358 +94,3 @@ 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_ALL(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;
- bool valid_unit_name;
-
- valid_unit_name = unit_name_is_valid(de->d_name, UNIT_NAME_ANY);
-
- /* We only care about valid units and dirs with certain suffixes, let's ignore the
- * rest. */
- if (!valid_unit_name &&
- !ENDSWITH_SET(de->d_name, ".wants", ".requires", ".d"))
- continue;
-
- 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 (!valid_unit_name)
- 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 (IN_SET(name_type, 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 (name_type == UNIT_NAME_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", template);
-
- 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_debug("%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 52e17f7..e57f472 100644
--- a/src/shared/unit-file.h
+++ b/src/shared/unit-file.h
@@ -3,12 +3,10 @@
#include <stdbool.h>
-#include "hashmap.h"
#include "unit-name.h"
typedef enum UnitFileState UnitFileState;
typedef enum UnitFileScope UnitFileScope;
-typedef struct LookupPaths LookupPaths;
enum UnitFileState {
UNIT_FILE_ENABLED,
@@ -39,16 +37,3 @@ 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 69063ee..48e0bad 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -33,7 +33,6 @@
#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"
@@ -167,18 +166,12 @@ 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);
@@ -2590,24 +2583,38 @@ static int unit_find_paths(
return log_error_errno(r, "Failed to get DropInPaths: %s", bus_error_message(&error, r));
}
} else {
- const char *_path;
- _cleanup_set_free_free_ Set *names = NULL;
+ _cleanup_set_free_ Set *names = NULL;
+ _cleanup_free_ char *template = NULL;
- if (!cached_name_map) {
- r = unit_file_build_name_map(lp, &cached_id_map, &cached_name_map, NULL);
- if (r < 0)
- return r;
- }
+ names = set_new(NULL);
+ if (!names)
+ return log_oom();
- r = unit_file_find_fragment(cached_id_map, cached_name_map, unit_name, &_path, &names);
+ r = unit_find_template_path(unit_name, lp, &path, &template);
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 (_path) {
- path = strdup(_path);
- if (!path)
- return log_oom();
- }
+ if (r < 0)
+ return log_error_errno(r, "Failed to add unit name: %m");
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 988ac15..b626b5f 100644
--- a/src/test/test-unit-file.c
+++ b/src/test/test-unit-file.c
@@ -25,50 +25,10 @@ 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(char **ids) {
- _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);
- }
-
- char **id;
- STRV_FOREACH(id, ids) {
- const char *fragment, *name;
- Iterator it;
- _cleanup_set_free_free_ Set *names = NULL;
- log_info("*** %s ***", *id);
- r = unit_file_find_fragment(unit_ids,
- unit_names,
- *id,
- &fragment,
- &names);
- assert(r == 0);
- log_info("fragment: %s", fragment);
- log_info("names:");
- SET_FOREACH(name, names, it)
- log_info(" %s", name);
- }
-}
-
int main(int argc, char **argv) {
test_setup_logging(LOG_DEBUG);
test_unit_validate_alias_symlink_and_warn();
- test_unit_file_build_name_map(strv_skip(argv, 1));
return 0;
}
diff --git a/test/TEST-15-DROPIN/test-dropin.sh b/test/TEST-15-DROPIN/test-dropin.sh
index 2cef5a3..f785680 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_ok bar@0 Names bar-alias@0
+ check_ko 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_ok bar@0 Requires bar-alias-0-requires.device
+ check_ko bar@0 Requires bar-alias-0-requires.device
check_ok bar@0 Requires bar-template-requires.device
- check_ok bar@0 Requires bar-alias-template-requires.device
+ check_ko 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_ok bar@1 Names bar-alias@1
+ check_ko 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_ok bar@1 Requires bar-alias-1-requires.device
+ check_ko 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_ok bar@1 Requires bar-alias-template-requires.device
+ check_ko 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_ko bar-alias@3 After bar-template-after.device
+ check_ok bar-alias@3 After bar-template-after.device
- check_ko bar-alias@3 Requires bar-3-requires.device
+ check_ok bar-alias@3 Requires bar-3-requires.device
check_ok bar-alias@3 Requires bar-alias-3-requires.device
- check_ko bar-alias@3 Requires bar-template-requires.device
+ check_ok bar-alias@3 Requires bar-template-requires.device
check_ok bar-alias@3 Requires bar-alias-template-requires.device
- check_ok bar-alias@3 Requires yup-template-requires.device
- check_ok bar-alias@3 Requires yup-3-requires.device
+ check_ko bar-alias@3 Requires yup-template-requires.device
+ check_ko bar-alias@3 Requires yup-3-requires.device
clear_services foo {bar,yup,bar-alias}@{,1,2,3}
}
@@ -267,7 +267,14 @@ test_alias_dropins () {
rm /etc/systemd/system/b1.service
clear_services a b
- # Check that dependencies don't vary.
+ # 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).
echo "*** test 2"
create_services a x y
mkdir -p /etc/systemd/system/a1.service.wants/
@@ -278,7 +285,7 @@ test_alias_dropins () {
check_ok a1 Wants y.service
systemctl start a
check_ok a1 Wants x.service # see [2]
- check_ok a1 Wants y.service
+ check_ko a1 Wants y.service
systemctl stop a x y
rm /etc/systemd/system/a1.service
--
1.8.3.1
Reference in New Issue View Git Blame Copy Permalink