From 5dd11ab5f36ce71138005941ea69458e169b3af6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Feb 2017 21:06:38 -0500 Subject: [PATCH 1/6] basic/dirent-util: allow suffix to be omitted for dirent_is_file_with_suffix --- src/basic/conf-files.c | 3 --- src/basic/dirent-util.c | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/basic/conf-files.c b/src/basic/conf-files.c index c0c22610d7..b5780194df 100644 --- a/src/basic/conf-files.c +++ b/src/basic/conf-files.c @@ -43,7 +43,6 @@ static int files_add(Hashmap *h, const char *root, const char *path, const char int r; assert(path); - assert(suffix); dirpath = prefix_roota(root, path); @@ -94,7 +93,6 @@ static int conf_files_list_strv_internal(char ***strv, const char *suffix, const int r; assert(strv); - assert(suffix); /* This alters the dirs string array */ if (!path_strv_resolve_uniq(dirs, root)) @@ -126,7 +124,6 @@ int conf_files_list_strv(char ***strv, const char *suffix, const char *root, con _cleanup_strv_free_ char **copy = NULL; assert(strv); - assert(suffix); copy = strv_copy((char**) dirs); if (!copy) diff --git a/src/basic/dirent-util.c b/src/basic/dirent-util.c index 59067121b7..6b9d26773e 100644 --- a/src/basic/dirent-util.c +++ b/src/basic/dirent-util.c @@ -70,5 +70,8 @@ bool dirent_is_file_with_suffix(const struct dirent *de, const char *suffix) { if (de->d_name[0] == '.') return false; + if (!suffix) + return true; + return endswith(de->d_name, suffix); } From 957787821008f5823804c928c9cdc77bb3910027 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 4 Feb 2017 20:50:44 -0500 Subject: [PATCH 2/6] core: when loading .wants and .requires, follow the same logic as .d conf dropins Essentially, instead of sequentially adding deps based on all symlinks encountered in .wants and .requires dirs for each name and each unit file load path, iteratate over the load paths and unit names gathering symlinks, then order them based on priority, and then iterate over the final list, adding dependencies. This patch doesn't change the logic too much, except that the order in which dependencies are applied might be different. It wasn't defined before, so that not really a change. Adding filtering on the symlinks is left for later patches. --- src/core/load-dropin.c | 71 +++++++++++++++++++++++++-------------- src/core/load-dropin.h | 10 +++--- src/shared/dropin.c | 11 +++--- src/shared/dropin.h | 26 ++++++++------ src/systemctl/systemctl.c | 3 +- 5 files changed, 75 insertions(+), 46 deletions(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index fc07151d37..0511fb38ef 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -22,50 +22,71 @@ #include "load-dropin.h" #include "load-fragment.h" #include "log.h" +#include "stat-util.h" #include "strv.h" #include "unit-name.h" #include "unit.h" -static int add_dependency_consumer( - UnitDependency dependency, - const char *entry, - const char* filepath, - void *arg) { - Unit *u = arg; +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_add_dependency_by_name(u, dependency, entry, filepath, true); + r = unit_file_find_dropin_paths(NULL, + u->manager->lookup_paths.search_path, + u->manager->unit_path_cache, + dir_suffix, + NULL, + u->names, + &paths); if (r < 0) - log_error_errno(r, "Cannot add dependency %s to %s, ignoring: %m", entry, u->id); + return r; + + STRV_FOREACH(p, paths) { + const char *entry; + + entry = basename(*p); + + 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; + } + + /* TODO: add more checks on the symlink name and target here */ + + r = unit_add_dependency_by_name(u, dependency, entry, *p, true); + if (r < 0) + log_unit_error_errno(u, r, "cannot add %s dependency on %s, ignoring: %m", + unit_dependency_to_string(dependency), entry); + } return 0; } int unit_load_dropin(Unit *u) { _cleanup_strv_free_ char **l = NULL; - Iterator i; - char *t, **f; + char **f; int r; assert(u); - /* Load dependencies from supplementary drop-in directories */ + /* Load dependencies from .wants and .requires directories */ + r = process_deps(u, UNIT_WANTS, ".wants"); + 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); - } - } + r = process_deps(u, UNIT_REQUIRES, ".requires"); + if (r < 0) + return r; + /* 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 319827dfb9..5828a223ce 100644 --- a/src/core/load-dropin.h +++ b/src/core/load-dropin.h @@ -25,11 +25,11 @@ /* Read service data supplementary drop-in directories */ 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, - u->names, - paths); + return unit_file_find_dropin_conf_paths(NULL, + u->manager->lookup_paths.search_path, + u->manager->unit_path_cache, + u->names, + paths); } int unit_load_dropin(Unit *u); diff --git a/src/shared/dropin.c b/src/shared/dropin.c index 06cf3de620..3944ccec44 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -173,7 +173,7 @@ static int iterate_dir( return 0; } -int unit_file_process_dir( +static int unit_file_process_dir( const char *original_root, Set *unit_path_cache, const char *unit_path, @@ -221,6 +221,8 @@ 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, Set *names, char ***paths) { @@ -235,16 +237,17 @@ int unit_file_find_dropin_paths( char **p; STRV_FOREACH(p, lookup_path) - unit_file_process_dir(original_root, unit_path_cache, *p, t, ".d", + unit_file_process_dir(original_root, unit_path_cache, + *p, t, dir_suffix, _UNIT_DEPENDENCY_INVALID, NULL, NULL, &strv); } if (strv_isempty(strv)) return 0; - r = conf_files_list_strv(&ans, ".conf", NULL, (const char**) strv); + r = conf_files_list_strv(&ans, file_suffix, NULL, (const char**) strv); if (r < 0) - return log_warning_errno(r, "Failed to get list of configuration files: %m"); + return log_warning_errno(r, "Failed to sort the list of configuration files: %m"); *paths = ans; ans = NULL; diff --git a/src/shared/dropin.h b/src/shared/dropin.h index 761b250886..22cd2fa229 100644 --- a/src/shared/dropin.h +++ b/src/shared/dropin.h @@ -44,20 +44,24 @@ typedef int (*dependency_consumer_t)(UnitDependency dependency, 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, Set *names, char ***paths); + +static inline int unit_file_find_dropin_conf_paths( + const char *original_root, + char **lookup_path, + Set *unit_path_cache, + Set *names, + char ***paths) { + return unit_file_find_dropin_paths(original_root, + lookup_path, + unit_path_cache, + ".d", ".conf", + names, paths); +} diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 2964b4e6b2..5cb07739d4 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2601,7 +2601,8 @@ static int unit_find_paths( return log_error_errno(r, "Failed to add unit name: %m"); if (dropin_paths) { - r = unit_file_find_dropin_paths(arg_root, lp->search_path, NULL, names, &dropins); + r = unit_file_find_dropin_conf_paths(arg_root, lp->search_path, + NULL, names, &dropins); if (r < 0) return r; } From 5be5d39bf0cda1026138bb6d7f43c03bb1f9e5cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 4 Feb 2017 20:50:44 -0500 Subject: [PATCH 3/6] core: implement masking of .wants/.requires symlinks Fixes #1169. Fixes #4830. Example log errors: Feb 04 22:13:28 systemd[1462]: foo.service: Wants dependency on empty_file.service is masked by /home/zbyszek/.config/systemd/user/foo.service.wants/empty_file.service, ignoring Feb 04 22:13:28 systemd[1462]: foo.service: Wants dependency on masked.service is masked by /home/zbyszek/.config/systemd/user/foo.service.wants/masked.service, ignoring --- src/core/load-dropin.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index 0511fb38ef..a43b6540ff 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -47,6 +47,13 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff 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", From dcc4f30ecce8c5af153e45663f928409095ba7ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 4 Feb 2017 21:32:08 -0500 Subject: [PATCH 4/6] core: drop code that is now unused --- src/shared/dropin.c | 85 +++++++++++++-------------------------------- src/shared/dropin.h | 11 ------ 2 files changed, 25 insertions(+), 71 deletions(-) diff --git a/src/shared/dropin.c b/src/shared/dropin.c index 3944ccec44..3917eb8f23 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -117,17 +117,12 @@ int write_drop_in_format(const char *dir, const char *unit, unsigned level, return write_drop_in(dir, unit, level, name, p); } -static int iterate_dir( - const char *path, +static int unit_file_find_dir( const char *original_root, - UnitDependency dependency, - dependency_consumer_t consumer, - void *arg, - char ***strv) { + const char *path, + char ***dirs) { _cleanup_free_ char *chased = NULL; - _cleanup_closedir_ DIR *d = NULL; - struct dirent *de; int r; assert(path); @@ -137,52 +132,21 @@ static int iterate_dir( 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) { - r = strv_push(strv, chased); - if (r < 0) - return log_oom(); - - chased = NULL; - return 0; - } - - 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; - } + r = strv_push(dirs, chased); + if (r < 0) + return log_oom(); + chased = NULL; return 0; } -static int unit_file_process_dir( +static int unit_file_find_dirs( 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) { + char ***dirs) { _cleanup_free_ char *path = NULL; int r; @@ -195,8 +159,11 @@ static int unit_file_process_dir( 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); + if (!unit_path_cache || set_get(unit_path_cache, path)) { + r = unit_file_find_dir(original_root, path, dirs); + if (r < 0) + return r; + } if (unit_name_is_valid(name, UNIT_NAME_INSTANCE)) { _cleanup_free_ char *template = NULL, *p = NULL; @@ -210,8 +177,11 @@ static int unit_file_process_dir( if (!p) return log_oom(); - if (!unit_path_cache || set_get(unit_path_cache, p)) - (void) iterate_dir(p, original_root, dependency, consumer, arg, strv); + if (!unit_path_cache || set_get(unit_path_cache, p)) { + r = unit_file_find_dir(original_root, p, dirs); + if (r < 0) + return r; + } } return 0; @@ -226,26 +196,21 @@ int unit_file_find_dropin_paths( Set *names, char ***paths) { - _cleanup_strv_free_ char **strv = NULL, **ans = NULL; + _cleanup_strv_free_ char **dirs = NULL, **ans = NULL; Iterator i; - char *t; + char *t, **p; int r; assert(paths); - SET_FOREACH(t, names, i) { - char **p; - + SET_FOREACH(t, names, i) STRV_FOREACH(p, lookup_path) - unit_file_process_dir(original_root, unit_path_cache, - *p, t, dir_suffix, - _UNIT_DEPENDENCY_INVALID, NULL, NULL, &strv); - } + unit_file_find_dirs(original_root, unit_path_cache, *p, t, dir_suffix, &dirs); - if (strv_isempty(strv)) + if (strv_isempty(dirs)) return 0; - r = conf_files_list_strv(&ans, file_suffix, NULL, (const char**) strv); + r = conf_files_list_strv(&ans, file_suffix, NULL, (const char**) dirs); if (r < 0) return log_warning_errno(r, "Failed to sort the list of configuration files: %m"); diff --git a/src/shared/dropin.h b/src/shared/dropin.h index 22cd2fa229..a2b8cdce61 100644 --- a/src/shared/dropin.h +++ b/src/shared/dropin.h @@ -33,17 +33,6 @@ 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_find_dropin_paths( const char *original_root, char **lookup_path, From 317cd27afb80161f4136d2c6e6f4d1685226dacd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 4 Feb 2017 22:36:17 -0500 Subject: [PATCH 5/6] core/load-dropin: add more sanity checks on .wants/.requires symlinks Feb 04 22:35:42 systemd[1462]: foo.service: Wants dependency dropin /home/zbyszek/.config/systemd/user/foo.service.wants/diffname.service target ../barbar.service has different name Feb 04 22:35:42 systemd[1462]: foo.service: Wants dependency dropin /home/zbyszek/.config/systemd/user/foo.service.wants/wrongname is not a valid unit name, ignoring --- src/core/load-dropin.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index a43b6540ff..ff3636149a 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -19,14 +19,37 @@ #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 bool unit_name_compatible(const char *a, const char *b) { + _cleanup_free_ char *prefix = NULL; + int r; + + /* the straightforward case: the symlink name matches the target */ + if (streq(a, b)) + return true; + + r = unit_name_template(a, &prefix); + if (r < 0) { + log_oom(); + return true; + } + + /* an instance name points to a target that is just the template name */ + if (streq(prefix, b)) + return true; + + return false; +} + static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suffix) { _cleanup_strv_free_ char **paths = NULL; char **p; @@ -44,6 +67,7 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff STRV_FOREACH(p, paths) { const char *entry; + _cleanup_free_ char *target = NULL; entry = basename(*p); @@ -66,7 +90,23 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff continue; } - /* TODO: add more checks on the symlink name and target here */ + 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. */ + if (!unit_name_compatible(entry, basename(target))) + 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, *p, true); if (r < 0) From fbc42f133232fb92140a8039b2d643f6f0b63889 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Fri, 27 Jan 2017 16:02:22 +0100 Subject: [PATCH 6/6] tests: add dropin dependency tests [zj: tests assertions adjusted to the different logic in which masking of a dependency through one name, does not forbid the dependency being added through another name.] --- test/TEST-15-DROPIN/Makefile | 1 + test/TEST-15-DROPIN/test-dropin.sh | 274 ++++++++++++++++++++++++++ test/TEST-15-DROPIN/test.sh | 46 +++++ test/TEST-15-DROPIN/testsuite.service | 7 + 4 files changed, 328 insertions(+) create mode 120000 test/TEST-15-DROPIN/Makefile create mode 100755 test/TEST-15-DROPIN/test-dropin.sh create mode 100755 test/TEST-15-DROPIN/test.sh create mode 100644 test/TEST-15-DROPIN/testsuite.service diff --git a/test/TEST-15-DROPIN/Makefile b/test/TEST-15-DROPIN/Makefile new file mode 120000 index 0000000000..e9f93b1104 --- /dev/null +++ b/test/TEST-15-DROPIN/Makefile @@ -0,0 +1 @@ +../TEST-01-BASIC/Makefile \ No newline at end of file diff --git a/test/TEST-15-DROPIN/test-dropin.sh b/test/TEST-15-DROPIN/test-dropin.sh new file mode 100755 index 0000000000..9d8af99ac4 --- /dev/null +++ b/test/TEST-15-DROPIN/test-dropin.sh @@ -0,0 +1,274 @@ +#! /bin/bash + +set -e +set -x + +_clear_service () { + systemctl stop $1.service 2>/dev/null || : + rm -f /{etc,run,usr/lib}/systemd/system/$1.service + rm -fr /{etc,run,usr/lib}/systemd/system/$1.service.d + rm -fr /{etc,run,usr/lib}/systemd/system/$1.service.{wants,requires} +} + +clear_services () { + for u in $*; do + _clear_service $u + done + systemctl daemon-reload +} + +create_service () { + clear_services $1 + + cat >/etc/systemd/system/$1.service</usr/lib/systemd/system/a.service.d/wants-b.conf</dev/null + + # mask some services that we do not want to run in these tests + ln -s /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -s /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -s /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -s /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -s /dev/null $initdir/etc/systemd/system/systemd-resolved.service + + # import the test scripts in the rootfs and plug them in systemd + cp testsuite.service $initdir/etc/systemd/system/ + cp test-dropin.sh $initdir/ + setup_testsuite + + # create dedicated rootfs for nspawn (located in $TESTDIR/nspawn-root) + setup_nspawn_root +} + +test_cleanup() { + return 0 +} + +do_test "$@" diff --git a/test/TEST-15-DROPIN/testsuite.service b/test/TEST-15-DROPIN/testsuite.service new file mode 100644 index 0000000000..d9790c2610 --- /dev/null +++ b/test/TEST-15-DROPIN/testsuite.service @@ -0,0 +1,7 @@ +[Unit] +Description=Testsuite service +After=multi-user.target + +[Service] +ExecStart=/test-dropin.sh +Type=oneshot