From 72406c2f7d23455d9e25a395cbf674cc4aff2149 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 22 Feb 2018 18:24:57 +0100 Subject: [PATCH 1/8] unit-name: add new unit_name_build_from_type() helper The new helper is much like unit_name_build() but expects a UnitType value instead of a suffix. --- src/basic/unit-name.c | 30 +++++++++++++++++++++++++----- src/basic/unit-name.h | 1 + src/test/test-unit-name.c | 21 +++++++++++++++++++++ 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/basic/unit-name.c b/src/basic/unit-name.c index 07a80829a5..6fb9cbc361 100644 --- a/src/basic/unit-name.c +++ b/src/basic/unit-name.c @@ -241,25 +241,45 @@ int unit_name_change_suffix(const char *n, const char *suffix, char **ret) { } int unit_name_build(const char *prefix, const char *instance, const char *suffix, char **ret) { - char *s; + UnitType type; assert(prefix); assert(suffix); assert(ret); + if (isempty(suffix)) + return -EINVAL; + if (suffix[0] != '.') + return -EINVAL; + + type = unit_type_from_string(suffix + 1); + if (type < 0) + return -EINVAL; + + return unit_name_build_from_type(prefix, instance, type, ret); +} + +int unit_name_build_from_type(const char *prefix, const char *instance, UnitType type, char **ret) { + const char *ut; + char *s; + + assert(prefix); + assert(type >= 0); + assert(type < _UNIT_TYPE_MAX); + assert(ret); + if (!unit_prefix_is_valid(prefix)) return -EINVAL; if (instance && !unit_instance_is_valid(instance)) return -EINVAL; - if (!unit_suffix_is_valid(suffix)) - return -EINVAL; + ut = unit_type_to_string(type); if (!instance) - s = strappend(prefix, suffix); + s = strjoin(prefix, ".", ut); else - s = strjoin(prefix, "@", instance, suffix); + s = strjoin(prefix, "@", instance, ".", ut); if (!s) return -ENOMEM; diff --git a/src/basic/unit-name.h b/src/basic/unit-name.h index aadaa85d73..d3e9d3e018 100644 --- a/src/basic/unit-name.h +++ b/src/basic/unit-name.h @@ -40,6 +40,7 @@ UnitType unit_name_to_type(const char *n) _pure_; int unit_name_change_suffix(const char *n, const char *suffix, char **ret); int unit_name_build(const char *prefix, const char *instance, const char *suffix, char **ret); +int unit_name_build_from_type(const char *prefix, const char *instance, UnitType, char **ret); char *unit_name_escape(const char *f); int unit_name_unescape(const char *f, char **ret); diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c index d71221228e..061adc70d8 100644 --- a/src/test/test-unit-name.c +++ b/src/test/test-unit-name.c @@ -449,6 +449,26 @@ static void test_unit_name_path_unescape(void) { test_unit_name_path_unescape_one("", NULL, -EINVAL); } +static void test_unit_name_to_prefix_one(const char *input, int ret, const char *output) { + _cleanup_free_ char *k = NULL; + + assert_se(unit_name_to_prefix(input, &k) == ret); + assert_se(streq_ptr(k, output)); +} + +static void test_unit_name_to_prefix(void) { + test_unit_name_to_prefix_one("foobar.service", 0, "foobar"); + test_unit_name_to_prefix_one("", -EINVAL, NULL); + test_unit_name_to_prefix_one("foobar", -EINVAL, NULL); + test_unit_name_to_prefix_one(".service", -EINVAL, NULL); + test_unit_name_to_prefix_one("quux.quux", -EINVAL, NULL); + test_unit_name_to_prefix_one("quux.mount", 0, "quux"); + test_unit_name_to_prefix_one("quux-quux.mount", 0, "quux-quux"); + test_unit_name_to_prefix_one("quux@bar.mount", 0, "quux"); + test_unit_name_to_prefix_one("quux-@.mount", 0, "quux-"); + test_unit_name_to_prefix_one("@.mount", -EINVAL, NULL); +} + int main(int argc, char* argv[]) { _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL; int r, rc = 0; @@ -482,6 +502,7 @@ int main(int argc, char* argv[]) { test_unit_name_escape(); test_unit_name_template(); test_unit_name_path_unescape(); + test_unit_name_to_prefix(); return rc; } From 77cc47d8b403341e9ecad7a67bb1c675cc8cec56 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Feb 2018 12:20:14 +0100 Subject: [PATCH 2/8] load-dropin: rename variable We are converting the unit name into its template, hence name the variable that way, instead of the misleading 'prefix'. --- src/core/load-dropin.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index 9fde34893d..854c2fd853 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -18,26 +18,21 @@ #include "unit.h" static int unit_name_compatible(const char *a, const char *b) { - _cleanup_free_ char *prefix = NULL; + _cleanup_free_ char *template = NULL; int r; - /* the straightforward case: the symlink name matches the target */ + /* The straightforward case: the symlink name matches the target */ if (streq(a, b)) return 1; - r = unit_name_template(a, &prefix); + r = unit_name_template(a, &template); if (r == -EINVAL) - /* not a template */ - return 0; + return 0; /* Not a template */ if (r < 0) - /* oom, or some other failure. Just skip the warning. */ - return r; + return r; /* OOM, or some other failure. Just skip the warning. */ - /* an instance name points to a target that is just the template name */ - if (streq(prefix, b)) - return 1; - - return 0; + /* 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) { From 952713b07a85fe1b1f645659b872abeaf71e2f11 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 21 Mar 2018 19:08:46 +0100 Subject: [PATCH 3/8] dropin: don't fully suppress logging about ENAMETOOLONG. Just downgrade to LOG_DEBUG --- src/shared/dropin.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/shared/dropin.c b/src/shared/dropin.c index 9544cab0b0..7719059624 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -115,14 +115,16 @@ static int unit_file_find_dir( assert(path); r = chase_symlinks(path, original_root, 0, &chased); - /* Ignore -ENOENT, after all most units won't have a drop-in dir. - * Also ignore -ENAMETOOLONG, users are not even able to create - * the drop-in dir in such case. This mostly happens for device units with long /sys path. - * */ - if (IN_SET(r, -ENOENT, -ENAMETOOLONG)) + 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); + return 0; + } if (r < 0) - return log_full_errno(LOG_WARNING, r, "Failed to canonicalize path %s: %m", path); + return log_warning_errno(r, "Failed to canonicalize path '%s': %m", path); r = strv_push(dirs, chased); if (r < 0) From b119facd27e3ab95cc5f1f33d14e29058fc4d51e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Feb 2018 12:30:14 +0100 Subject: [PATCH 4/8] core: minor coding style changes --- src/core/load-dropin.c | 2 +- src/core/load-dropin.h | 2 ++ src/shared/dropin.h | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index 854c2fd853..0a56f0df86 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -51,8 +51,8 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff return r; STRV_FOREACH(p, paths) { - const char *entry; _cleanup_free_ char *target = NULL; + const char *entry; entry = basename(*p); diff --git a/src/core/load-dropin.h b/src/core/load-dropin.h index c97de616f1..583460e3dd 100644 --- a/src/core/load-dropin.h +++ b/src/core/load-dropin.h @@ -13,6 +13,8 @@ /* Read service data supplementary drop-in directories */ static inline int unit_find_dropin_paths(Unit *u, char ***paths) { + assert(u); + return unit_file_find_dropin_conf_paths(NULL, u->manager->lookup_paths.search_path, u->manager->unit_path_cache, diff --git a/src/shared/dropin.h b/src/shared/dropin.h index a14fcc023f..fa6f5402f9 100644 --- a/src/shared/dropin.h +++ b/src/shared/dropin.h @@ -36,6 +36,7 @@ static inline int unit_file_find_dropin_conf_paths( Set *unit_path_cache, Set *names, char ***paths) { + return unit_file_find_dropin_paths(original_root, lookup_path, unit_path_cache, From eb2c3192dc6f4f4d86023f9379e1fb964a3c6c8c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Feb 2018 12:31:30 +0100 Subject: [PATCH 5/8] systemctl: fix indentation in output of "systemcl status" if there are multiple drop-in dirs We were a few whitespace off. Let's fix that. --- src/systemctl/systemctl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 079d571857..388eee47a1 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -4021,8 +4021,9 @@ static void print_status_info( char ** dropin; STRV_FOREACH(dropin, i->dropin_paths) { - if (! dir || last) { - printf(dir ? " " : " Drop-In: "); + if (!dir || last) { + printf(dir ? " " : + " Drop-In: "); dir = mfree(dir); @@ -4032,7 +4033,8 @@ static void print_status_info( return; } - printf("%s\n %s", dir, + printf("%s\n" + " %s", dir, special_glyph(TREE_RIGHT)); } From 5396624506e155c4bc10c0ee65b939600860ab67 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Feb 2018 12:32:23 +0100 Subject: [PATCH 6/8] dropin: when looking for dropins for a unit, also look within "-" prefix unit dirs This extends the logic by which we look for drop-ins for unit files when loading them. Previously for a unit "foo-quux-bar.service" we'd look in a directory "foo-quux-bar.service.d" accompanying it for extension dropins. With this change we'll additionally look in: "foo-quux-.service.d" and "foo-.service.d", i.e. we'll truncate the unit name after every dash. This is an alternative to templating for many services, as it permits configuring defaults for sets of units that all use the same prefix in the unit name. This is particularly useful in slice, mount and automount units which reflect a hierarchy of concepts, as it permits setting defaults for specific subsets of the tree. For example, in order to provide every user with a memory of 1G it's now possible to do: # mkdir -p /etc/systemd/system/user-.slice.d # cat > /etc/systemd/system/user-.slice.d/50-memory.conf << EOF [Slice] MemoryMax=1G EOF # systemctl daemon-reload This makes use of the fact that every user gets his own slice unit when logging in, named "user-$UID.slice". This doesn't precisely provide what is requested in #2556, but it does provide equivalent functionality. Fixes: #2556 See: #3504 #7599 --- src/shared/dropin.c | 71 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 63 insertions(+), 8 deletions(-) diff --git a/src/shared/dropin.c b/src/shared/dropin.c index 7719059624..d9362f6919 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -142,7 +142,12 @@ static int unit_file_find_dirs( 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; int r; assert(unit_path); @@ -150,26 +155,76 @@ static int unit_file_find_dirs( assert(suffix); path = strjoina(unit_path, "/", name, suffix); - 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)) { - /* Also try the template dir */ - + is_instance = unit_name_is_valid(name, UNIT_NAME_INSTANCE); + if (is_instance) { /* Also try the template dir */ _cleanup_free_ char *template = NULL; r = unit_name_template(name, &template); if (r < 0) return log_error_errno(r, "Failed to generate template from unit name: %m"); - return unit_file_find_dirs(original_root, unit_path_cache, unit_path, template, suffix, dirs); + r = unit_file_find_dirs(original_root, unit_path_cache, unit_path, template, suffix, dirs); + if (r < 0) + return r; } - 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; + } + + /* Trailing dash? If so, chop it off and try again, but not more than once. */ + prefix[n] = 0; + chopped = true; + } + + if (!unit_prefix_is_valid(prefix)) + return 0; + + type = unit_name_to_type(name); + if (type < 0) { + log_error("Failed to to derive unit type from unit name: %m"); + return -EINVAL; + } + + 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); } int unit_file_find_dropin_paths( @@ -182,15 +237,15 @@ int unit_file_find_dropin_paths( char ***ret) { _cleanup_strv_free_ char **dirs = NULL; - Iterator i; char *t, **p; + Iterator i; int r; assert(ret); SET_FOREACH(t, names, i) STRV_FOREACH(p, lookup_path) - unit_file_find_dirs(original_root, unit_path_cache, *p, t, dir_suffix, &dirs); + (void) unit_file_find_dirs(original_root, unit_path_cache, *p, t, dir_suffix, &dirs); if (strv_isempty(dirs)) { *ret = NULL; From 7ce49e656b1377713ade999dfe381807a78313cd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 21 Mar 2018 19:55:54 +0100 Subject: [PATCH 7/8] test: add test for prefix unit loading --- src/test/test-engine.c | 11 ++++++++++- test/meson.build | 13 +++++++++---- test/unit-.service.d/10-override.conf | 2 ++ test/unit-with-.service.d/20-override.conf | 2 ++ test/unit-with-multiple-.service.d/20-override.conf | 2 ++ test/unit-with-multiple-.service.d/30-override.conf | 2 ++ test/unit-with-multiple-dashes.service | 6 ++++++ .../10-override.conf | 2 ++ 8 files changed, 35 insertions(+), 5 deletions(-) create mode 100644 test/unit-.service.d/10-override.conf create mode 100644 test/unit-with-.service.d/20-override.conf create mode 100644 test/unit-with-multiple-.service.d/20-override.conf create mode 100644 test/unit-with-multiple-.service.d/30-override.conf create mode 100644 test/unit-with-multiple-dashes.service create mode 100644 test/unit-with-multiple-dashes.service.d/10-override.conf diff --git a/src/test/test-engine.c b/src/test/test-engine.c index 566874a299..1c12a8644d 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -19,12 +19,16 @@ int main(int argc, char *argv[]) { _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL; _cleanup_(sd_bus_error_free) sd_bus_error err = SD_BUS_ERROR_NULL; _cleanup_(manager_freep) Manager *m = NULL; - Unit *a = NULL, *b = NULL, *c = NULL, *d = NULL, *e = NULL, *g = NULL, *h = NULL; + Unit *a = NULL, *b = NULL, *c = NULL, *d = NULL, *e = NULL, *g = NULL, *h = NULL, *unit_with_multiple_dashes = NULL; FILE *serial = NULL; FDSet *fdset = NULL; Job *j; int r; + log_set_max_level(LOG_DEBUG); + log_parse_environment(); + log_open(); + r = enter_cgroup_subroot(); if (r == -ENOMEDIUM) { log_notice_errno(r, "Skipping test: cgroupfs not available"); @@ -130,5 +134,10 @@ int main(int argc, char *argv[]) { assert_se(!hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], c)); assert_se(!hashmap_get(c->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); + assert_se(manager_load_unit(m, "unit-with-multiple-dashes.service", NULL, NULL, &unit_with_multiple_dashes) >= 0); + + assert_se(strv_equal(unit_with_multiple_dashes->documentation, STRV_MAKE("man:test", "man:override2", "man:override3"))); + assert_se(streq_ptr(unit_with_multiple_dashes->description, "override4")); + return 0; } diff --git a/test/meson.build b/test/meson.build index 0bd45cd891..c112e09577 100644 --- a/test/meson.build +++ b/test/meson.build @@ -158,13 +158,18 @@ test_data_files = ''' test-path/path-unit.path test-path/paths.target test-path/sysinit.target - testsuite.target - timers.target - unstoppable.service test-umount/empty.mountinfo + test-umount/example.swaps test-umount/garbled.mountinfo test-umount/rhbug-1554943.mountinfo - test-umount/example.swaps + testsuite.target + timers.target + unit-with-.service.d/20-override.conf + unit-with-multiple-.service.d/20-override.conf + unit-with-multiple-.service.d/30-override.conf + unit-with-multiple-dashes.service + unit-with-multiple-dashes.service.d/10-override.conf + unstoppable.service '''.split() if conf.get('ENABLE_RESOLVE') == 1 diff --git a/test/unit-.service.d/10-override.conf b/test/unit-.service.d/10-override.conf new file mode 100644 index 0000000000..916737d415 --- /dev/null +++ b/test/unit-.service.d/10-override.conf @@ -0,0 +1,2 @@ +[Unit] +Description=override0 diff --git a/test/unit-with-.service.d/20-override.conf b/test/unit-with-.service.d/20-override.conf new file mode 100644 index 0000000000..c6c2438f73 --- /dev/null +++ b/test/unit-with-.service.d/20-override.conf @@ -0,0 +1,2 @@ +[Unit] +Documentation=man:override1 diff --git a/test/unit-with-multiple-.service.d/20-override.conf b/test/unit-with-multiple-.service.d/20-override.conf new file mode 100644 index 0000000000..62fafd2e3b --- /dev/null +++ b/test/unit-with-multiple-.service.d/20-override.conf @@ -0,0 +1,2 @@ +[Unit] +Documentation=man:override2 diff --git a/test/unit-with-multiple-.service.d/30-override.conf b/test/unit-with-multiple-.service.d/30-override.conf new file mode 100644 index 0000000000..b9616da8a8 --- /dev/null +++ b/test/unit-with-multiple-.service.d/30-override.conf @@ -0,0 +1,2 @@ +[Unit] +Documentation=man:override3 diff --git a/test/unit-with-multiple-dashes.service b/test/unit-with-multiple-dashes.service new file mode 100644 index 0000000000..b38b3604b8 --- /dev/null +++ b/test/unit-with-multiple-dashes.service @@ -0,0 +1,6 @@ +[Unit] +Description=A unit with multiple dashes +Documentation=man:test + +[Service] +ExecStart=/bin/true diff --git a/test/unit-with-multiple-dashes.service.d/10-override.conf b/test/unit-with-multiple-dashes.service.d/10-override.conf new file mode 100644 index 0000000000..982c3621a6 --- /dev/null +++ b/test/unit-with-multiple-dashes.service.d/10-override.conf @@ -0,0 +1,2 @@ +[Unit] +Description=override4 From 6c0a77953c13cd55504c5a9968ce8b3044f72408 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Feb 2018 20:42:57 +0100 Subject: [PATCH 8/8] man: document the new dash truncation drop-in directories --- man/systemd.unit.xml | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 9cabc3c4f8..2baf9d17e8 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -189,15 +189,24 @@ suffix is .requires/ in this case. Along with a unit file foo.service, a "drop-in" directory - foo.service.d/ may exist. All files with the suffix - .conf from this directory will be parsed after the file itself is - parsed. This is useful to alter or add configuration settings for a unit, without having to - modify unit files. Each drop-in file must have appropriate section headers. Note that for - instantiated units, this logic will first look for the instance .d/ - subdirectory and read its .conf files, followed by the template - .d/ subdirectory and the .conf files there. + foo.service.d/ may exist. All files with the suffix .conf from this + directory will be parsed after the unit file itself is parsed. This is useful to alter or add configuration + settings for a unit, without having to modify unit files. Drop-in files must contain appropriate section + headers. For instantiated units, this logic will first look for the instance .d/ subdirectory + (e.g. foo@bar.service.d/) and read its .conf files, followed by the template + .d/ subdirectory (e.g. foo@.service.d/) and the .conf + files there. Moreover for units names containing dashes (-), the set of directories generated by + truncating the unit name after all dashes is searched too. Specifically, for a unit name + foo-bar-baz.service not only the the regular drop-in directory + foo-bar-baz.service.d/ is searched but also both foo-bar-.service.d/ and + foo-.service.d/. This is useful for defining common drop-ins for a set of related units, whose + names begin with a common prefix. This scheme is particularly useful for mount, automount and slice units, whose + systematic naming structure is built around dashes as component separators. Note that equally named drop-in files + further down the prefix hierarchy override those further up, + i.e. foo-bar-.service.d/10-override.conf overrides + foo-.service.d/10-override.conf. - In addition to /etc/systemd/system, the drop-in .d + In addition to /etc/systemd/system, the drop-in .d/ directories for system services can be placed in /usr/lib/systemd/system or /run/systemd/system directories. Drop-in files in /etc take precedence over those in /run which in turn take precedence over those