From 08178b2bddf3b2e03eee907414155f4217fa6191 Mon Sep 17 00:00:00 2001 From: ingi2-kim Date: Mon, 8 Jun 2020 17:11:34 +0900 Subject: [PATCH] Revert: Mask individual .wants/.requires symlinks Revert below patches due to performance issue (Avoid increasing IO count) Refer : https://github.com/systemd/systemd/pull/5231 - core/load-dropin: add more sanity checks on .wants/.requires symlinks - core: drop code that is now unused - core: implement masking of .wants/.requires symlinks - core: when loading .wants and .requires, follow the same logic as .d conf dropins Change-Id: I9f6712d9df2c6bb25ab736ae6b6d1f5adbf2a691 Signed-off-by: ingi2-kim --- src/core/load-dropin.c | 118 ++++++---------------------- src/core/load-dropin.h | 1 - src/shared/dropin.c | 196 +++++++++++++++++----------------------------- src/shared/dropin.h | 24 +++++- src/systemctl/systemctl.c | 4 +- 5 files changed, 121 insertions(+), 222 deletions(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index a50b200..42e1537 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -1,123 +1,53 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include "conf-parser.h" -#include "fs-util.h" #include "load-dropin.h" #include "load-fragment.h" #include "log.h" -#include "stat-util.h" -#include "string-util.h" #include "strv.h" #include "unit-name.h" #include "unit.h" -static int unit_name_compatible(const char *a, const char *b) { - _cleanup_free_ char *template = NULL; +static int add_dependency_consumer( + UnitDependency dependency, + const char *entry, + const char* filepath, + void *arg) { + Unit *u = arg; 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; - int r; + assert(u); - r = unit_file_find_dropin_paths(NULL, - u->manager->lookup_paths.search_path, - u->manager->unit_path_cache, - dir_suffix, - NULL, - u->names, - &paths); + r = unit_add_dependency_by_name(u, dependency, entry, filepath, true); if (r < 0) - return r; - - STRV_FOREACH(p, paths) { - _cleanup_free_ char *target = NULL; - const char *entry; - - entry = basename(*p); - - if (null_or_empty_path(*p) > 0) { - /* an error usually means an invalid symlink, which is not a mask */ - log_unit_debug(u, "%s dependency on %s is masked by %s, ignoring.", - unit_dependency_to_string(dependency), entry, *p); - continue; - } - - r = is_symlink(*p); - if (r < 0) { - log_unit_warning_errno(u, r, "%s dropin %s unreadable, ignoring: %m", - unit_dependency_to_string(dependency), *p); - continue; - } - if (r == 0) { - log_unit_warning(u, "%s dependency dropin %s is not a symlink, ignoring.", - unit_dependency_to_string(dependency), *p); - continue; - } - - if (!unit_name_is_valid(entry, UNIT_NAME_ANY)) { - log_unit_warning(u, "%s dependency dropin %s is not a valid unit name, ignoring.", - unit_dependency_to_string(dependency), *p); - continue; - } - - r = readlink_malloc(*p, &target); - if (r < 0) { - log_unit_warning_errno(u, r, "readlink(\"%s\") failed, ignoring: %m", *p); - continue; - } - - /* 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)); - if (r < 0) { - 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) - log_unit_warning(u, "%s dependency dropin %s target %s has different name", - unit_dependency_to_string(dependency), *p, target); - - r = unit_add_dependency_by_name(u, dependency, entry, true, UNIT_DEPENDENCY_FILE); - if (r < 0) - log_unit_warning_errno(u, r, "Cannot add %s dependency on %s, ignoring: %m", - unit_dependency_to_string(dependency), entry); - } + log_error_errno(r, "Cannot add dependency %s to %s, ignoring: %m", entry, u->id); return 0; } int unit_load_dropin(Unit *u) { _cleanup_strv_free_ char **l = NULL; - char **f; + Iterator i; + char *t, **f; int r; assert(u); - /* Load dependencies from .wants and .requires directories */ - r = process_deps(u, UNIT_WANTS, ".wants"); - if (r < 0) - return r; + /* Load dependencies from supplementary drop-in directories */ - r = process_deps(u, UNIT_REQUIRES, ".requires"); - if (r < 0) - return r; + SET_FOREACH(t, u->names, i) { + char **p; + + STRV_FOREACH(p, u->manager->lookup_paths.search_path) { + unit_file_process_dir(NULL, u->manager->unit_path_cache, *p, t, + ".wants", UNIT_WANTS, + add_dependency_consumer, u, NULL); + unit_file_process_dir(NULL, u->manager->unit_path_cache, *p, t, + ".requires", UNIT_REQUIRES, + add_dependency_consumer, u, NULL); + } + } - /* Load .conf dropins */ r = unit_find_dropin_paths(u, &l); if (r <= 0) return 0; diff --git a/src/core/load-dropin.h b/src/core/load-dropin.h index ea15554..b9df150 100644 --- a/src/core/load-dropin.h +++ b/src/core/load-dropin.h @@ -12,7 +12,6 @@ static inline int unit_find_dropin_paths(Unit *u, char ***paths) { return unit_file_find_dropin_paths(NULL, u->manager->lookup_paths.search_path, u->manager->unit_path_cache, - ".d", ".conf", u->names, paths); } diff --git a/src/shared/dropin.c b/src/shared/dropin.c index ca97285..20686e8 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -93,182 +93,134 @@ int write_drop_in_format(const char *dir, const char *unit, unsigned level, return write_drop_in(dir, unit, level, name, p); } -static int unit_file_add_dir( - const char *original_root, +static int iterate_dir( const char *path, - char ***dirs) { + const char *original_root, + UnitDependency dependency, + dependency_consumer_t consumer, + void *arg, + char ***strv) { _cleanup_free_ char *chased = NULL; + _cleanup_closedir_ DIR *d = NULL; + struct dirent *de; int r; assert(path); - /* This adds [original_root]/path to dirs, if it exists. */ - r = chase_symlinks(path, original_root, 0, &chased, NULL); - if (r == -ENOENT) /* Ignore -ENOENT, after all most units won't have a drop-in dir. */ - return 0; - if (r == -ENAMETOOLONG) { - /* Also, ignore -ENAMETOOLONG but log about it. After all, users are not even able to create the - * drop-in dir in such case. This mostly happens for device units with an overly long /sys path. */ - log_debug_errno(r, "Path '%s' too long, couldn't canonicalize, ignoring.", path); + if (r < 0) + return log_full_errno(r == -ENOENT ? LOG_DEBUG : LOG_WARNING, + r, "Failed to canonicalize path %s: %m", path); + + /* The config directories are special, since the order of the + * drop-ins matters */ + if (dependency < 0) { + if (strv_consume(strv, TAKE_PTR(chased)) < 0) + return log_oom(); + return 0; } - if (r < 0) - return log_warning_errno(r, "Failed to canonicalize path '%s': %m", path); - if (strv_consume(dirs, TAKE_PTR(chased)) < 0) - return log_oom(); + assert(consumer); + + d = opendir(chased); + if (!d) { + if (errno == ENOENT) + return 0; + + return log_warning_errno(errno, "Failed to open directory %s: %m", path); + } + + FOREACH_DIRENT(de, d, return log_warning_errno(errno, "Failed to read directory %s: %m", path)) { + _cleanup_free_ char *f = NULL; + + f = strjoin(path, "/", de->d_name); + if (!f) + return log_oom(); + + r = consumer(dependency, de->d_name, f, arg); + if (r < 0) + return r; + } return 0; } -static int unit_file_find_dirs( +int unit_file_process_dir( const char *original_root, Set *unit_path_cache, const char *unit_path, const char *name, const char *suffix, - char ***dirs) { - - _cleanup_free_ char *prefix = NULL, *instance = NULL, *built = NULL; - bool is_instance, chopped; - const char *dash; - UnitType type; - char *path; - size_t n; + UnitDependency dependency, + dependency_consumer_t consumer, + void *arg, + char ***strv) { + + _cleanup_free_ char *path = NULL; int r; assert(unit_path); assert(name); assert(suffix); - path = strjoina(unit_path, "/", name, suffix); - if (!unit_path_cache || set_get(unit_path_cache, path)) { - r = unit_file_add_dir(original_root, path, dirs); - if (r < 0) - return r; - } + path = strjoin(unit_path, "/", name, suffix); + if (!path) + return log_oom(); + + if (!unit_path_cache || set_get(unit_path_cache, path)) + (void) iterate_dir(path, original_root, dependency, consumer, arg, strv); - is_instance = unit_name_is_valid(name, UNIT_NAME_INSTANCE); - if (is_instance) { /* Also try the template dir */ - _cleanup_free_ char *template = NULL; + if (unit_name_is_valid(name, UNIT_NAME_INSTANCE)) { + _cleanup_free_ char *template = NULL, *p = NULL; + /* Also try the template dir */ r = unit_name_template(name, &template); if (r < 0) return log_error_errno(r, "Failed to generate template from unit name: %m"); - r = unit_file_find_dirs(original_root, unit_path_cache, unit_path, template, suffix, dirs); - if (r < 0) - return r; - } - - /* Return early for top level drop-ins. */ - if (unit_type_from_string(name) >= 0) - return 0; - - /* Let's see if there's a "-" prefix for this unit name. If so, let's invoke ourselves for it. This will then - * recursively do the same for all our prefixes. i.e. this means given "foo-bar-waldo.service" we'll also - * search "foo-bar-.service" and "foo-.service". - * - * Note the order in which we do it: we traverse up adding drop-ins on each step. This means the more specific - * drop-ins may override the more generic drop-ins, which is the intended behaviour. */ - - r = unit_name_to_prefix(name, &prefix); - if (r < 0) - return log_error_errno(r, "Failed to derive unit name prefix from unit name: %m"); - - chopped = false; - for (;;) { - dash = strrchr(prefix, '-'); - if (!dash) /* No dash? if so we are done */ - return 0; - - n = (size_t) (dash - prefix); - if (n == 0) /* Leading dash? If so, we are done */ - return 0; - - if (prefix[n+1] != 0 || chopped) { - prefix[n+1] = 0; - break; - } + p = strjoin(unit_path, "/", template, suffix); + if (!p) + return log_oom(); - /* Trailing dash? If so, chop it off and try again, but not more than once. */ - prefix[n] = 0; - chopped = true; + if (!unit_path_cache || set_get(unit_path_cache, p)) + (void) iterate_dir(p, original_root, dependency, consumer, arg, strv); } - if (!unit_prefix_is_valid(prefix)) - return 0; - - type = unit_name_to_type(name); - if (type < 0) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Failed to to derive unit type from unit name: %s", - name); - - if (is_instance) { - r = unit_name_to_instance(name, &instance); - if (r < 0) - return log_error_errno(r, "Failed to derive unit name instance from unit name: %m"); - } - - r = unit_name_build_from_type(prefix, instance, type, &built); - if (r < 0) - return log_error_errno(r, "Failed to build prefix unit name: %m"); - - return unit_file_find_dirs(original_root, unit_path_cache, unit_path, built, suffix, dirs); + return 0; } int unit_file_find_dropin_paths( const char *original_root, char **lookup_path, Set *unit_path_cache, - const char *dir_suffix, - const char *file_suffix, const Set *names, - char ***ret) { + char ***paths) { - _cleanup_strv_free_ char **dirs = NULL; - UnitType type = _UNIT_TYPE_INVALID; + _cleanup_strv_free_ char **strv = NULL, **ans = NULL; char *name, **p; Iterator i; int r; - assert(ret); + assert(paths); - /* All the names in the unit are of the same type so just grab one. */ - name = (char*) set_first(names); - if (name) { - type = unit_name_to_type(name); - if (type < 0) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Failed to to derive unit type from unit name: %s", - name); - } - - /* Special top level drop in for ".". Add this first as it's the most generic - * and should be able to be overridden by more specific drop-ins. */ - STRV_FOREACH(p, lookup_path) - (void) unit_file_find_dirs(original_root, - unit_path_cache, - *p, - unit_type_to_string(type), - dir_suffix, - &dirs); - - SET_FOREACH(name, names, i) + SET_FOREACH(name, names, i) { STRV_FOREACH(p, lookup_path) - (void) unit_file_find_dirs(original_root, unit_path_cache, *p, name, dir_suffix, &dirs); + (void) unit_file_process_dir(original_root, unit_path_cache, *p, name, ".d", + _UNIT_DEPENDENCY_INVALID, NULL, NULL, &strv); + } - if (strv_isempty(dirs)) { - *ret = NULL; + if (strv_isempty(strv)) { + *paths = NULL; return 0; } - r = conf_files_list_strv(ret, file_suffix, NULL, 0, (const char**) dirs); + r = conf_files_list_strv(paths, ".conf", NULL, 0, (const char**) strv); if (r < 0) - return log_warning_errno(r, "Failed to create the list of configuration files: %m"); + return log_warning_errno(r, "Failed to get list of configuration files: %m"); + *paths = ans; + ans = NULL; return 1; } diff --git a/src/shared/dropin.h b/src/shared/dropin.h index 89a2ab1..fe0e5d8 100644 --- a/src/shared/dropin.h +++ b/src/shared/dropin.h @@ -15,11 +15,31 @@ int write_drop_in(const char *dir, const char *unit, unsigned level, int write_drop_in_format(const char *dir, const char *unit, unsigned level, const char *name, const char *format, ...) _printf_(5, 6); +/** + * This callback will be called for each directory entry @entry, + * with @filepath being the full path to the entry. + * + * If return value is negative, loop will be aborted. + */ +typedef int (*dependency_consumer_t)(UnitDependency dependency, + const char *entry, + const char* filepath, + void *arg); + +int unit_file_process_dir( + const char *original_root, + Set * unit_path_cache, + const char *unit_path, + const char *name, + const char *suffix, + UnitDependency dependency, + dependency_consumer_t consumer, + void *arg, + char ***strv); + int unit_file_find_dropin_paths( const char *original_root, char **lookup_path, Set *unit_path_cache, - const char *dir_suffix, - const char *file_suffix, const Set *names, char ***paths); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 6335238..275bac0 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2682,9 +2682,7 @@ static int unit_find_paths( 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, - ".d", ".conf", - names, &dropins); + r = unit_file_find_dropin_paths(arg_root, lp->search_path, NULL, names, &dropins); if (r < 0) return r; } -- 2.7.4