From e5369d1a8fc1771a2af68656b37e60286c28038e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 27 Jul 2019 13:55:29 +0200 Subject: [PATCH 1/8] shared/install: typo --- src/shared/install.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/install.c b/src/shared/install.c index dfb6036191..8f9cf4a2f4 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -2723,7 +2723,7 @@ int unit_file_lookup_state( break; default: - assert_not_reached("Unexpect unit file type."); + assert_not_reached("Unexpected unit file type."); } *ret = state; From 976ed3b621034a061940c3f74cb24a236adf01cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 30 Jul 2019 09:33:58 +0200 Subject: [PATCH 2/8] test: use "ln -fs" Without this, repeated runs of "make -C TEST/... setup" fail when trying to create the symlink. --- test/TEST-06-SELINUX/test.sh | 10 +++++----- test/TEST-08-ISSUE-2730/test.sh | 10 +++++----- test/TEST-09-ISSUE-2691/test.sh | 10 +++++----- test/TEST-10-ISSUE-2467/test.sh | 10 +++++----- test/TEST-14-MACHINE-ID/test.sh | 10 +++++----- test/TEST-15-DROPIN/test.sh | 10 +++++----- test/TEST-16-EXTEND-TIMEOUT/test.sh | 10 +++++----- test/TEST-17-UDEV-WANTS/test.sh | 10 +++++----- test/TEST-18-FAILUREACTION/test.sh | 10 +++++----- test/TEST-24-UNIT-TESTS/test.sh | 6 +++--- test/TEST-28-PERCENTJ-WANTEDBY/test.sh | 10 +++++----- test/TEST-29-UDEV-ID_RENAMING/test.sh | 10 +++++----- test/TEST-31-DEVICE-ENUMERATION/test.sh | 10 +++++----- 13 files changed, 63 insertions(+), 63 deletions(-) diff --git a/test/TEST-06-SELINUX/test.sh b/test/TEST-06-SELINUX/test.sh index 4a6cf126ab..2e983f7879 100755 --- a/test/TEST-06-SELINUX/test.sh +++ b/test/TEST-06-SELINUX/test.sh @@ -93,11 +93,11 @@ EOF ) # 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 + ln -fs /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -fs /dev/null $initdir/etc/systemd/system/systemd-resolved.service } do_test "$@" diff --git a/test/TEST-08-ISSUE-2730/test.sh b/test/TEST-08-ISSUE-2730/test.sh index 6c5edd4d2f..8643842e81 100755 --- a/test/TEST-08-ISSUE-2730/test.sh +++ b/test/TEST-08-ISSUE-2730/test.sh @@ -65,11 +65,11 @@ EOF ln -s /etc/systemd/system/-.mount $initdir/etc/systemd/system/local-fs.target.wants/-.mount # 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 + ln -fs /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -fs /dev/null $initdir/etc/systemd/system/systemd-resolved.service } do_test "$@" diff --git a/test/TEST-09-ISSUE-2691/test.sh b/test/TEST-09-ISSUE-2691/test.sh index 9bc53d5e69..1a8b900672 100755 --- a/test/TEST-09-ISSUE-2691/test.sh +++ b/test/TEST-09-ISSUE-2691/test.sh @@ -33,11 +33,11 @@ EOF ) # 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 + ln -fs /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -fs /dev/null $initdir/etc/systemd/system/systemd-resolved.service } do_test "$@" diff --git a/test/TEST-10-ISSUE-2467/test.sh b/test/TEST-10-ISSUE-2467/test.sh index bb4c0f3ac4..2c29514ef9 100755 --- a/test/TEST-10-ISSUE-2467/test.sh +++ b/test/TEST-10-ISSUE-2467/test.sh @@ -47,11 +47,11 @@ EOF setup_nspawn_root # 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 + ln -fs /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -fs /dev/null $initdir/etc/systemd/system/systemd-resolved.service } do_test "$@" diff --git a/test/TEST-14-MACHINE-ID/test.sh b/test/TEST-14-MACHINE-ID/test.sh index 81e76f381f..e52377c79d 100755 --- a/test/TEST-14-MACHINE-ID/test.sh +++ b/test/TEST-14-MACHINE-ID/test.sh @@ -74,11 +74,11 @@ chmod +x $initdir/test-machine-id-setup.sh ) # 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 + ln -fs /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -fs /dev/null $initdir/etc/systemd/system/systemd-resolved.service } do_test "$@" diff --git a/test/TEST-15-DROPIN/test.sh b/test/TEST-15-DROPIN/test.sh index 47a4067276..9d5ea138b9 100755 --- a/test/TEST-15-DROPIN/test.sh +++ b/test/TEST-15-DROPIN/test.sh @@ -10,11 +10,11 @@ test_setup() { setup_basic_environment # 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 + ln -fs /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -fs /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/ diff --git a/test/TEST-16-EXTEND-TIMEOUT/test.sh b/test/TEST-16-EXTEND-TIMEOUT/test.sh index 016a877b96..e128dc7969 100755 --- a/test/TEST-16-EXTEND-TIMEOUT/test.sh +++ b/test/TEST-16-EXTEND-TIMEOUT/test.sh @@ -30,11 +30,11 @@ test_setup() { setup_testsuite ) # 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 + ln -fs /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -fs /dev/null $initdir/etc/systemd/system/systemd-resolved.service setup_nspawn_root } diff --git a/test/TEST-17-UDEV-WANTS/test.sh b/test/TEST-17-UDEV-WANTS/test.sh index 065fa9624f..8727432e30 100755 --- a/test/TEST-17-UDEV-WANTS/test.sh +++ b/test/TEST-17-UDEV-WANTS/test.sh @@ -16,11 +16,11 @@ test_setup() { setup_basic_environment # 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 + ln -fs /dev/null $initdir/etc/systemd/system/systemd-hwdb-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-journal-catalog-update.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.service + ln -fs /dev/null $initdir/etc/systemd/system/systemd-networkd.socket + ln -fs /dev/null $initdir/etc/systemd/system/systemd-resolved.service # setup the testsuite service cat >$initdir/etc/systemd/system/testsuite.service <$initdir/etc/systemd/system/testsuite.service <$initdir/etc/systemd/system/testsuite.service < Date: Tue, 30 Jul 2019 12:28:48 +0200 Subject: [PATCH 3/8] TEST-15-DROPIN: add test for details of unit aliasing I adjusted the tests to pass. I don't think the behaviour makes much sense, even if we ignore the issue with "lazy loading" of aliases. E.g. in the last section, the fact that dropins for yup@.service and yup@3.service are not loaded seems to be a plain old bug. --- test/TEST-15-DROPIN/test-dropin.sh | 140 ++++++++++++++++++++++++++++- 1 file changed, 139 insertions(+), 1 deletion(-) diff --git a/test/TEST-15-DROPIN/test-dropin.sh b/test/TEST-15-DROPIN/test-dropin.sh index 02962a8a07..f7856807c4 100755 --- a/test/TEST-15-DROPIN/test-dropin.sh +++ b/test/TEST-15-DROPIN/test-dropin.sh @@ -109,10 +109,148 @@ test_template_dropins () { create_services foo bar@ yup@ + # Declare some deps to check if the body was loaded + cat >>/etc/systemd/system/bar@.service <>/etc/systemd/system/yup@.service < Date: Tue, 2 Apr 2019 11:22:56 +0200 Subject: [PATCH 4/8] shared/unit-file: add a function to validate unit alias symlinks It turns out most possible symlinks are invalid, because the type has to match, and template units can only be linked to template units. I'm not sure if the existing code made the same checks consistently. At least I don't see the same rules expressed in a single place. --- src/shared/unit-file.c | 73 +++++++++++++++++++++++++++++++++++++++ src/shared/unit-file.h | 2 ++ src/test/meson.build | 4 +++ src/test/test-unit-file.c | 33 ++++++++++++++++++ 4 files changed, 112 insertions(+) create mode 100644 src/test/test-unit-file.c diff --git a/src/shared/unit-file.c b/src/shared/unit-file.c index deed7dcf09..cde38c472b 100644 --- a/src/shared/unit-file.c +++ b/src/shared/unit-file.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include "macro.h" +#include "string-util.h" #include "unit-file.h" bool unit_type_may_alias(UnitType type) { @@ -21,3 +22,75 @@ bool unit_type_may_template(UnitType type) { UNIT_TIMER, UNIT_PATH); } + +int unit_validate_alias_symlink_and_warn(const char *filename, const char *target) { + const char *src, *dst; + _cleanup_free_ char *src_instance = NULL, *dst_instance = NULL; + UnitType src_unit_type, dst_unit_type; + int src_name_type, dst_name_type; + + /* Check if the *alias* symlink is valid. This applies to symlinks like + * /etc/systemd/system/dbus.service → dbus-broker.service, but not to .wants or .requires symlinks + * and such. Neither does this apply to symlinks which *link* units, i.e. symlinks to outside of the + * unit lookup path. + * + * -EINVAL is returned if the something is wrong with the source filename or the source unit type is + * not allowed to symlink, + * -EXDEV if the target filename is not a valid unit name or doesn't match the source. + */ + + src = basename(filename); + dst = basename(target); + + /* src checks */ + + src_name_type = unit_name_to_instance(src, &src_instance); + if (src_name_type < 0) + return log_notice_errno(src_name_type, + "%s: not a valid unit name \"%s\": %m", filename, src); + + src_unit_type = unit_name_to_type(src); + assert(src_unit_type >= 0); /* unit_name_to_instance() checked the suffix already */ + + if (!unit_type_may_alias(src_unit_type)) + return log_notice_errno(SYNTHETIC_ERRNO(EINVAL), + "%s: symlinks are not allowed for units of this type, rejecting.", + filename); + + if (src_name_type != UNIT_NAME_PLAIN && + !unit_type_may_template(src_unit_type)) + return log_notice_errno(SYNTHETIC_ERRNO(EINVAL), + "%s: templates not allowed for %s units, rejecting.", + filename, unit_type_to_string(src_unit_type)); + + /* dst checks */ + + dst_name_type = unit_name_to_instance(dst, &dst_instance); + if (dst_name_type < 0) + return log_notice_errno(dst_name_type == -EINVAL ? SYNTHETIC_ERRNO(EXDEV) : dst_name_type, + "%s points to \"%s\" which is not a valid unit name: %m", + filename, dst); + + if (!(dst_name_type == src_name_type || + (src_name_type == UNIT_NAME_INSTANCE && dst_name_type == UNIT_NAME_TEMPLATE))) + return log_notice_errno(SYNTHETIC_ERRNO(EXDEV), + "%s: symlink target name type \"%s\" does not match source, rejecting.", + filename, dst); + + if (dst_name_type == UNIT_NAME_INSTANCE) { + assert(src_instance); + assert(dst_instance); + if (!streq(src_instance, dst_instance)) + return log_notice_errno(SYNTHETIC_ERRNO(EXDEV), + "%s: unit symlink target \"%s\" instance name doesn't match, rejecting.", + filename, dst); + } + + dst_unit_type = unit_name_to_type(dst); + if (dst_unit_type != src_unit_type) + return log_notice_errno(SYNTHETIC_ERRNO(EXDEV), + "%s: symlink target \"%s\" has incompatible suffix, rejecting.", + filename, dst); + + return 0; +} diff --git a/src/shared/unit-file.h b/src/shared/unit-file.h index 2b9df655ee..e57f472f4f 100644 --- a/src/shared/unit-file.h +++ b/src/shared/unit-file.h @@ -35,3 +35,5 @@ enum UnitFileScope { 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); diff --git a/src/test/meson.build b/src/test/meson.build index ddc04dda65..c25ecf62fc 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -137,6 +137,10 @@ tests += [ [], 'ENABLE_EFI'], + [['src/test/test-unit-file.c'], + [], + []], + [['src/test/test-unit-name.c', 'src/test/test-helper.c'], [libcore, diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c new file mode 100644 index 0000000000..5e281b28d5 --- /dev/null +++ b/src/test/test-unit-file.c @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ + +#include "path-lookup.h" +#include "strv.h" +#include "tests.h" +#include "unit-file.h" + +static void test_unit_validate_alias_symlink_and_warn(void) { + log_info("/* %s */", __func__); + + assert_se(unit_validate_alias_symlink_and_warn("/path/a.service", "/other/b.service") == 0); + assert_se(unit_validate_alias_symlink_and_warn("/path/a.service", "/other/b.socket") == -EXDEV); + assert_se(unit_validate_alias_symlink_and_warn("/path/a.service", "/other/b.foobar") == -EXDEV); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@.service", "/other/b@.service") == 0); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@.service", "/other/b@.socket") == -EXDEV); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@XXX.service", "/other/b@YYY.service") == -EXDEV); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@XXX.service", "/other/b@YYY.socket") == -EXDEV); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@.service", "/other/b@YYY.service") == -EXDEV); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@XXX.service", "/other/b@XXX.service") == 0); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@XXX.service", "/other/b@.service") == 0); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@.service", "/other/b.service") == -EXDEV); + assert_se(unit_validate_alias_symlink_and_warn("/path/a.service", "/other/b@.service") == -EXDEV); + assert_se(unit_validate_alias_symlink_and_warn("/path/a@.slice", "/other/b.slice") == -EINVAL); + assert_se(unit_validate_alias_symlink_and_warn("/path/a.slice", "/other/b.slice") == -EINVAL); +} + +int main(int argc, char **argv) { + test_setup_logging(LOG_DEBUG); + + test_unit_validate_alias_symlink_and_warn(); + + return 0; +} From e8630e695232bdfcd16b55f3faafb4329c961104 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 18 Jul 2019 13:11:28 +0200 Subject: [PATCH 5/8] 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 | 374 +++++++---------------------- 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, 532 insertions(+), 390 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 3288b0b838..7521d599e9 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 */ + /* 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; + other = manager_get_unit((*u)->manager, k); + if (!other) + return r; /* return previous failure */ - r = unit_name_replace_instance(k, (*u)->instance, &instance); - if (r < 0) - return r; + r = unit_merge(other, *u); + if (r < 0) + return r; - 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; + *u = other; + return merge_by_names(u, names, NULL); } - if (id == k) + if (streq_ptr(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); @@ -4814,78 +4611,79 @@ 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); + 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; + + if (fragment) { + /* Open the file, check if this is a mask, otherwise read. */ + _cleanup_fclose_ FILE *f = NULL; + struct stat st; + + /* 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 (fstat(fileno(f), &st) < 0) + return -errno; + + r = free_and_strdup(&u->fragment_path, fragment); + 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; + } + } + + /* 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; + + 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_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; - /* 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 (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); - } - - /* Look for a template */ - if (u->load_state == UNIT_STUB && u->instance) { - _cleanup_free_ char *k = NULL; - - r = unit_name_template(u->id, &k); - if (r < 0) - return r; - - 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; - } - - 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; - - r = load_from_path(u, z); - if (r < 0) - return r; - - if (u->load_state != UNIT_STUB) - break; - } - } - } + if (merged != u) + u->load_state = UNIT_MERGED; return 0; } diff --git a/src/core/manager.c b/src/core/manager.c index c176309edf..f8a7e7d64e 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 9879082fea..3cb37b3bf4 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 fa89bd4a4d..7561f1e1a1 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 cde38c472b..90fa949d24 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 e57f472f4f..52e17f7cfb 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 97f3176cc5..31d364cefe 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 5e281b28d5..c5144a1b7e 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 f7856807c4..2cef5a3c5b 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 From e67cd21d7d174cdafd12beca4cfb6e19e61f6fb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 8 Jul 2019 17:33:25 +0200 Subject: [PATCH 6/8] analyze: add "unit-files" to dump the unit fragment map I'm not convinced that this is useful enough to be included... But it is certainly nice when debugging. --- src/analyze/analyze.c | 50 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 92727974d6..a28b27a3a0 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -1545,6 +1545,53 @@ static int get_or_set_log_target(int argc, char *argv[], void *userdata) { return (argc == 1) ? get_log_target(argc, argv, userdata) : set_log_target(argc, argv, userdata); } +static bool strv_fnmatch_strv_or_empty(char* const* patterns, char **strv, int flags) { + char **s; + STRV_FOREACH(s, strv) + if (strv_fnmatch_or_empty(patterns, *s, flags)) + return true; + + return false; +} + +static int do_unit_files(int argc, char *argv[], void *userdata) { + _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_hashmap_free_ Hashmap *unit_ids = NULL; + _cleanup_hashmap_free_ Hashmap *unit_names = NULL; + char **patterns = strv_skip(argv, 1); + Iterator i; + const char *k, *dst; + char **v; + int r; + + r = lookup_paths_init(&lp, arg_scope, 0, NULL); + if (r < 0) + return log_error_errno(r, "lookup_paths_init() failed: %m"); + + r = unit_file_build_name_map(&lp, &unit_ids, &unit_names, NULL); + if (r < 0) + return log_error_errno(r, "unit_file_build_name_map() failed: %m"); + + HASHMAP_FOREACH_KEY(dst, k, unit_ids, i) { + if (!strv_fnmatch_or_empty(patterns, k, FNM_NOESCAPE) && + !strv_fnmatch_or_empty(patterns, dst, FNM_NOESCAPE)) + continue; + + printf("ids: %s → %s\n", k, dst); + } + + HASHMAP_FOREACH_KEY(v, k, unit_names, i) { + if (!strv_fnmatch_or_empty(patterns, k, FNM_NOESCAPE) && + !strv_fnmatch_strv_or_empty(patterns, v, FNM_NOESCAPE)) + continue; + + _cleanup_free_ char *j = strv_join(v, ", "); + printf("aliases: %s ← %s\n", k, j); + } + + return 0; +} + static int dump_unit_paths(int argc, char *argv[], void *userdata) { _cleanup_(lookup_paths_free) LookupPaths paths = {}; int r; @@ -2164,6 +2211,7 @@ static int help(int argc, char *argv[], void *userdata) { " log-target [TARGET] Get/set logging target for manager\n" " dump Output state serialization of service manager\n" " cat-config Show configuration file and drop-ins\n" + " unit-files List files and symlinks for units\n" " unit-paths List load directories for units\n" " syscall-filter [NAME...] Print list of syscalls in seccomp filter\n" " condition CONDITION... Evaluate conditions and asserts\n" @@ -2365,8 +2413,10 @@ static int run(int argc, char *argv[]) { { "get-log-level", VERB_ANY, 1, 0, get_log_level }, { "set-log-target", 2, 2, 0, set_log_target }, { "get-log-target", VERB_ANY, 1, 0, get_log_target }, + { "dump", VERB_ANY, 1, 0, dump }, { "cat-config", 2, VERB_ANY, 0, cat_config }, + { "unit-files", VERB_ANY, VERB_ANY, 0, do_unit_files }, { "unit-paths", 1, 1, 0, dump_unit_paths }, { "syscall-filter", VERB_ANY, VERB_ANY, 0, dump_syscall_filters }, { "condition", 2, VERB_ANY, 0, do_condition }, From 91e0ee5f16321656ed6f827742ecbeb2b36027f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 10 Jul 2019 18:01:13 +0200 Subject: [PATCH 7/8] pid1: drop unit caches only based on mtime v2: - do not watch mtime of transient and generated dirs We'd reload the map after every transient unit we created, which we don't need to do, since we create those units ourselves and know their fragment path. --- src/analyze/analyze.c | 2 +- src/core/load-fragment.c | 9 +++++++ src/core/manager.c | 14 ++-------- src/core/manager.h | 1 + src/shared/unit-file.c | 57 ++++++++++++++++++++++++++++++++++++++- src/shared/unit-file.h | 2 ++ src/systemctl/systemctl.c | 2 +- src/test/test-unit-file.c | 12 ++++++++- 8 files changed, 83 insertions(+), 16 deletions(-) diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index a28b27a3a0..4ebce83b2c 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -1568,7 +1568,7 @@ static int do_unit_files(int argc, char *argv[], void *userdata) { if (r < 0) return log_error_errno(r, "lookup_paths_init() failed: %m"); - r = unit_file_build_name_map(&lp, &unit_ids, &unit_names, NULL); + r = unit_file_build_name_map(&lp, NULL, &unit_ids, &unit_names, NULL); if (r < 0) return log_error_errno(r, "unit_file_build_name_map() failed: %m"); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 7521d599e9..f34e424fbc 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -4611,6 +4611,15 @@ int unit_load_fragment(Unit *u) { return 0; } + /* Possibly rebuild the fragment map to catch new units */ + r = unit_file_build_name_map(&u->manager->lookup_paths, + &u->manager->unit_cache_mtime, + &u->manager->unit_id_map, + &u->manager->unit_name_map, + &u->manager->unit_path_cache); + if (r < 0) + log_error_errno(r, "Failed to rebuild name map: %m"); + r = unit_file_find_fragment(u->manager->unit_id_map, u->manager->unit_name_map, u->id, diff --git a/src/core/manager.c b/src/core/manager.c index f8a7e7d64e..4a503a944e 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -678,6 +678,7 @@ 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); + m->unit_cache_mtime = 0; } static int manager_setup_run_queue(Manager *m) { @@ -1626,11 +1627,6 @@ 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_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 */ _cleanup_(manager_reloading_stopp) Manager *reloading = NULL; @@ -2847,10 +2843,6 @@ 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 - manager_check_finished(m); /* There might still be some zombies hanging around from before we were exec()'ed. Let's reap them. */ @@ -3526,10 +3518,8 @@ int manager_reload(Manager *m) { if (r < 0) log_warning_errno(r, "Failed to reduce unit file paths, ignoring: %m"); + /* We flushed out generated files, for which we don't watch mtime, so we should flush the old map. */ 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 3cb37b3bf4..596220848f 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -224,6 +224,7 @@ struct Manager { Hashmap *unit_id_map; Hashmap *unit_name_map; Set *unit_path_cache; + usec_t unit_cache_mtime; char **transient_environment; /* The environment, as determined from config files, kernel cmdline and environment generators */ char **client_environment; /* Environment variables created by clients through the bus API */ diff --git a/src/shared/unit-file.c b/src/shared/unit-file.c index 90fa949d24..8a09f3827f 100644 --- a/src/shared/unit-file.c +++ b/src/shared/unit-file.c @@ -152,8 +152,47 @@ static int unit_ids_map_get( return -ELOOP; } +static bool lookup_paths_mtime_exclude(const LookupPaths *lp, const char *path) { + /* Paths that are under our exclusive control. Users shall not alter those directly. */ + + return streq_ptr(path, lp->generator) || + streq_ptr(path, lp->generator_early) || + streq_ptr(path, lp->generator_late) || + streq_ptr(path, lp->transient) || + streq_ptr(path, lp->persistent_control) || + streq_ptr(path, lp->runtime_control); +} + +static bool lookup_paths_mtime_good(const LookupPaths *lp, usec_t mtime) { + char **dir; + + STRV_FOREACH(dir, (char**) lp->search_path) { + struct stat st; + + if (lookup_paths_mtime_exclude(lp, *dir)) + continue; + + /* Determine the latest lookup path modification time */ + if (stat(*dir, &st) < 0) { + if (errno == ENOENT) + continue; + + log_debug_errno(errno, "Failed to stat %s, ignoring: %m", *dir); + continue; + } + + if (timespec_load(&st.st_mtim) > mtime) { + log_debug_errno(errno, "Unit dir %s has changed, need to update cache.", *dir); + return false; + } + } + + return true; +} + int unit_file_build_name_map( const LookupPaths *lp, + usec_t *cache_mtime, Hashmap **ret_unit_ids_map, Hashmap **ret_unit_names_map, Set **ret_path_cache) { @@ -171,6 +210,12 @@ int unit_file_build_name_map( _cleanup_set_free_free_ Set *paths = NULL; char **dir; int r; + usec_t mtime = 0; + + /* Before doing anything, check if the mtime that was passed is still valid. If + * yes, do nothing. If *cache_time == 0, always build the cache. */ + if (cache_mtime && *cache_mtime > 0 && lookup_paths_mtime_good(lp, *cache_mtime)) + return 0; if (ret_path_cache) { paths = set_new(&path_hash_ops); @@ -181,6 +226,7 @@ int unit_file_build_name_map( STRV_FOREACH(dir, (char**) lp->search_path) { struct dirent *de; _cleanup_closedir_ DIR *d = NULL; + struct stat st; d = opendir(*dir); if (!d) { @@ -189,6 +235,13 @@ int unit_file_build_name_map( continue; } + /* Determine the latest lookup path modification time */ + if (fstat(dirfd(d), &st) < 0) + return log_error_errno(errno, "Failed to fstat %s: %m", *dir); + + if (!lookup_paths_mtime_exclude(lp, *dir)) + mtime = MAX(mtime, timespec_load(&st.st_mtim)); + 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; @@ -316,12 +369,14 @@ int unit_file_build_name_map( basename(dst), src); } + if (cache_mtime) + *cache_mtime = mtime; *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; + return 1; } int unit_file_find_fragment( diff --git a/src/shared/unit-file.h b/src/shared/unit-file.h index 52e17f7cfb..54cc7876fe 100644 --- a/src/shared/unit-file.h +++ b/src/shared/unit-file.h @@ -4,6 +4,7 @@ #include #include "hashmap.h" +#include "time-util.h" #include "unit-name.h" typedef enum UnitFileState UnitFileState; @@ -42,6 +43,7 @@ int unit_validate_alias_symlink_and_warn(const char *filename, const char *targe int unit_file_build_name_map( const LookupPaths *lp, + usec_t *ret_time, Hashmap **ret_unit_ids_map, Hashmap **ret_unit_names_map, Set **ret_path_cache); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 31d364cefe..1140deb0d1 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2593,7 +2593,7 @@ static int unit_find_paths( _cleanup_set_free_free_ Set *names = NULL; if (!cached_name_map) { - r = unit_file_build_name_map(lp, &cached_id_map, &cached_name_map, NULL); + r = unit_file_build_name_map(lp, NULL, &cached_id_map, &cached_name_map, NULL); if (r < 0) return r; } diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index c5144a1b7e..0c0371375a 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -31,10 +31,12 @@ static void test_unit_file_build_name_map(void) { Iterator i; const char *k, *dst; char **v; + usec_t mtime = 0; + int r; 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); + assert_se(unit_file_build_name_map(&lp, &mtime, &unit_ids, &unit_names, NULL) == 1); HASHMAP_FOREACH_KEY(dst, k, unit_ids, i) log_info("ids: %s → %s", k, dst); @@ -43,6 +45,14 @@ static void test_unit_file_build_name_map(void) { _cleanup_free_ char *j = strv_join(v, ", "); log_info("aliases: %s ← %s", k, j); } + + char buf[FORMAT_TIMESTAMP_MAX]; + log_debug("Last modification time: %s", format_timestamp(buf, sizeof buf, mtime)); + + r = unit_file_build_name_map(&lp, &mtime, &unit_ids, &unit_names, NULL); + assert_se(IN_SET(r, 0, 1)); + if (r == 0) + log_debug("Cache rebuild skipped based on mtime."); } int main(int argc, char **argv) { From 802765438ff7491a9452a4fd4fb68bb0597539c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 30 Jul 2019 12:29:18 +0200 Subject: [PATCH 8/8] test-unit-file: allow printing of information about specific units Useful for manual debugging. --- src/test/test-unit-file.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index 0c0371375a..8bc5bf6038 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include "path-lookup.h" +#include "set.h" #include "strv.h" #include "tests.h" #include "unit-file.h" @@ -24,7 +25,7 @@ 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) { +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; @@ -53,13 +54,32 @@ static void test_unit_file_build_name_map(void) { assert_se(IN_SET(r, 0, 1)); if (r == 0) log_debug("Cache rebuild skipped based on mtime."); + + + 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(); + test_unit_file_build_name_map(strv_skip(argv, 1)); return 0; }