From aa45911b793255bec34fe8c128c80bda1482cc14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 8 Jun 2021 09:06:11 +0200 Subject: [PATCH 1/5] man/50-xdg-data-dirs: add quotes as suggested by shellcheck --- man/50-xdg-data-dirs.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/man/50-xdg-data-dirs.sh b/man/50-xdg-data-dirs.sh index 89e9fbb599..ce062e394b 100755 --- a/man/50-xdg-data-dirs.sh +++ b/man/50-xdg-data-dirs.sh @@ -5,8 +5,8 @@ XDG_DATA_DIRS="${XDG_DATA_DIRS:-/usr/local/share/:/usr/share}" # add a directory if it exists if [[ -d /opt/foo/share ]]; then - XDG_DATA_DIRS=/opt/foo/share:${XDG_DATA_DIRS} + XDG_DATA_DIRS="/opt/foo/share:${XDG_DATA_DIRS}" fi # write our output -echo XDG_DATA_DIRS=$XDG_DATA_DIRS +echo "XDG_DATA_DIRS=${XDG_DATA_DIRS}" From a79726113a40e60d8f464b1e5c60de0e9fccf506 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 2 Jun 2021 11:27:55 +0200 Subject: [PATCH 2/5] TODO: elide initrd-parse-etc.service if possible --- TODO | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/TODO b/TODO index e75f111bb6..c87f978070 100644 --- a/TODO +++ b/TODO @@ -885,6 +885,10 @@ Features: * fstab-generator: default to tmpfs-as-root if only usr= is specified on the kernel cmdline +* initrd-parse-etc.service: can we skip daemon-reload if /sysroot/etc/fstab is missing? + Note that we start initrd-fs.target and initrd-cleanup.target there, so a straightforward + ConditionPathExists= is not enough. + * docs: bring http://www.freedesktop.org/wiki/Software/systemd/MyServiceCantGetRealtime up to date * add a job mode that will fail if a transaction would mean stopping From 6d216bdd07995f35139fa03829ae05a83a15c281 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 23 Jun 2021 16:05:47 +0200 Subject: [PATCH 3/5] test-path-util: check that dot components are irrelevant for path comparisons --- src/test/test-path-util.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c index 50aacdb967..4c041cd57f 100644 --- a/src/test/test-path-util.c +++ b/src/test/test-path-util.c @@ -126,6 +126,8 @@ static void test_path_compare_one(const char *a, const char *b, int expected) { } static void test_path_compare(void) { + log_info("/* %s */", __func__); + test_path_compare_one("/goo", "/goo", 0); test_path_compare_one("/goo", "/goo", 0); test_path_compare_one("//goo", "/goo", 0); @@ -138,6 +140,12 @@ static void test_path_compare(void) { test_path_compare_one("/x", "x/", 1); test_path_compare_one("x/", "/", -1); test_path_compare_one("/x/./y", "x/y", 1); + test_path_compare_one("/x/./y", "/x/y", 0); + test_path_compare_one("/x/./././y", "/x/y/././.", 0); + test_path_compare_one("./x/./././y", "./x/y/././.", 0); + test_path_compare_one(".", "./.", 0); + test_path_compare_one(".", "././.", 0); + test_path_compare_one("./..", ".", 1); test_path_compare_one("x/.y", "x/y", -1); test_path_compare_one("foo", "/foo", -1); test_path_compare_one("/foo", "/foo/bar", -1); From 0fb789af20d2d76262472925229a6f45fe0f92b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 23 Jun 2021 16:22:53 +0200 Subject: [PATCH 4/5] test-hash-funcs: add new file to test that path set ignores dot components --- src/test/meson.build | 2 + src/test/test-hash-funcs.c | 83 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 src/test/test-hash-funcs.c diff --git a/src/test/meson.build b/src/test/meson.build index 29f488f4d8..90aca5dcba 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -366,6 +366,8 @@ tests += [ [], [threads]], + [['src/test/test-hash-funcs.c']], + [['src/test/test-bitmap.c']], [['src/test/test-xml.c']], diff --git a/src/test/test-hash-funcs.c b/src/test/test-hash-funcs.c new file mode 100644 index 0000000000..db68e3a1e7 --- /dev/null +++ b/src/test/test-hash-funcs.c @@ -0,0 +1,83 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "tests.h" +#include "hash-funcs.h" +#include "set.h" + +static void test_path_hash_set(void) { + /* The goal is to make sure that non-simplified path are hashed as expected, + * and that we don't need to simplify them beforehand. */ + + log_info("/* %s */", __func__); + + /* No freeing of keys, we operate on static strings here… */ + _cleanup_set_free_ Set *set = NULL; + + assert_se(set_isempty(set)); + assert_se(set_ensure_put(&set, &path_hash_ops, "foo") == 1); + assert_se(set_ensure_put(&set, &path_hash_ops, "foo") == 0); + assert_se(set_ensure_put(&set, &path_hash_ops, "bar") == 1); + assert_se(set_ensure_put(&set, &path_hash_ops, "bar") == 0); + assert_se(set_ensure_put(&set, &path_hash_ops, "/foo") == 1); + assert_se(set_ensure_put(&set, &path_hash_ops, "/bar") == 1); + assert_se(set_ensure_put(&set, &path_hash_ops, "/foo/.") == 0); + assert_se(set_ensure_put(&set, &path_hash_ops, "/./bar/./.") == 0); + + assert_se(set_contains(set, "foo")); + assert_se(set_contains(set, "bar")); + assert_se(set_contains(set, "./foo")); + assert_se(set_contains(set, "./foo/.")); + assert_se(set_contains(set, "./bar")); + assert_se(set_contains(set, "./bar/.")); + assert_se(set_contains(set, "/foo")); + assert_se(set_contains(set, "/bar")); + assert_se(set_contains(set, "//./foo")); + assert_se(set_contains(set, "///./foo/.")); + assert_se(set_contains(set, "////./bar")); + assert_se(set_contains(set, "/////./bar/.")); + + assert_se(set_contains(set, "foo/")); + assert_se(set_contains(set, "bar/")); + assert_se(set_contains(set, "./foo/")); + assert_se(set_contains(set, "./foo/./")); + assert_se(set_contains(set, "./bar/")); + assert_se(set_contains(set, "./bar/./")); + assert_se(set_contains(set, "/foo/")); + assert_se(set_contains(set, "/bar/")); + assert_se(set_contains(set, "//./foo/")); + assert_se(set_contains(set, "///./foo/./")); + assert_se(set_contains(set, "////./bar/")); + assert_se(set_contains(set, "/////./bar/./")); + + assert_se(!set_contains(set, "foo.")); + assert_se(!set_contains(set, ".bar")); + assert_se(!set_contains(set, "./foo.")); + assert_se(!set_contains(set, "./.foo/.")); + assert_se(!set_contains(set, "../bar")); + assert_se(!set_contains(set, "./bar/..")); + assert_se(!set_contains(set, "./foo..")); + assert_se(!set_contains(set, "/..bar")); + assert_se(!set_contains(set, "//../foo")); + assert_se(!set_contains(set, "///../foo/.")); + assert_se(!set_contains(set, "////../bar")); + assert_se(!set_contains(set, "/////../bar/.")); + + assert_se(!set_contains(set, "foo./")); + assert_se(!set_contains(set, ".bar/")); + assert_se(!set_contains(set, "./foo./")); + assert_se(!set_contains(set, "./.foo/./")); + assert_se(!set_contains(set, "../bar/")); + assert_se(!set_contains(set, "./bar/../")); + assert_se(!set_contains(set, "./foo../")); + assert_se(!set_contains(set, "/..bar/")); + assert_se(!set_contains(set, "//../foo/")); + assert_se(!set_contains(set, "///../foo/./")); + assert_se(!set_contains(set, "////../bar/")); + assert_se(!set_contains(set, "/////../bar/./")); +} + +int main(int argc, char **argv) { + test_setup_logging(LOG_INFO); + + test_path_hash_set(); +} From ac19bdd04b376c12787b75428bf3d6caf43805ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 23 Jun 2021 17:32:15 +0200 Subject: [PATCH 5/5] core: avoid calling path_simplify() unnecessarilly for u.requires_mounts_for keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We would always call path_simplify() before doing a lookup, which requires the path key to be duplicated first. But the hashmap lookup doesn't require this… So let's opportunistically skip the allocation if the key is already present. Inspired by https://github.com/systemd/systemd/pull/19973. --- src/core/manager.c | 10 ++++------ src/core/unit.c | 30 ++++++++++++++---------------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 21358a0469..f3275a4070 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -4511,16 +4511,14 @@ void manager_status_printf(Manager *m, StatusType type, const char *status, cons va_end(ap); } -Set *manager_get_units_requiring_mounts_for(Manager *m, const char *path) { - char p[strlen(path)+1]; - +Set* manager_get_units_requiring_mounts_for(Manager *m, const char *path) { assert(m); assert(path); - strcpy(p, path); - path_simplify(p); + if (path_equal(path, "/")) + path = ""; - return hashmap_get(m->units_requiring_mounts_for, streq(p, "/") ? "" : p); + return hashmap_get(m->units_requiring_mounts_for, path); } int manager_update_failed_units(Manager *m, Unit *u, bool failed) { diff --git a/src/core/unit.c b/src/core/unit.c index 0cfb2aa281..3792cee711 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4562,45 +4562,43 @@ int unit_kill_context( } int unit_require_mounts_for(Unit *u, const char *path, UnitDependencyMask mask) { - _cleanup_free_ char *p = NULL; - UnitDependencyInfo di; int r; assert(u); assert(path); - /* Registers a unit for requiring a certain path and all its prefixes. We keep a hashtable of these paths in - * the unit (from the path to the UnitDependencyInfo structure indicating how to the dependency came to - * be). However, we build a prefix table for all possible prefixes so that new appearing mount units can easily - * determine which units to make themselves a dependency of. */ + /* Registers a unit for requiring a certain path and all its prefixes. We keep a hashtable of these + * paths in the unit (from the path to the UnitDependencyInfo structure indicating how to the + * dependency came to be). However, we build a prefix table for all possible prefixes so that new + * appearing mount units can easily determine which units to make themselves a dependency of. */ if (!path_is_absolute(path)) return -EINVAL; - r = hashmap_ensure_allocated(&u->requires_mounts_for, &path_hash_ops); - if (r < 0) - return r; + if (hashmap_contains(u->requires_mounts_for, path)) /* Exit quickly if the path is already covered. */ + return 0; - p = strdup(path); + _cleanup_free_ char *p = strdup(path); if (!p) return -ENOMEM; + /* Use the canonical form of the path as the stored key. We call path_is_normalized() + * only after simplification, since path_is_normalized() rejects paths with '.'. + * path_is_normalized() also verifies that the path fits in PATH_MAX. */ path = path_simplify(p); if (!path_is_normalized(path)) return -EPERM; - if (hashmap_contains(u->requires_mounts_for, path)) - return 0; - - di = (UnitDependencyInfo) { + UnitDependencyInfo di = { .origin_mask = mask }; - r = hashmap_put(u->requires_mounts_for, path, di.data); + r = hashmap_ensure_put(&u->requires_mounts_for, &path_hash_ops, p, di.data); if (r < 0) return r; - p = NULL; + assert(r > 0); + TAKE_PTR(p); /* path remains a valid pointer to the string stored in the hashmap */ char prefix[strlen(path) + 1]; PATH_FOREACH_PREFIX_MORE(prefix, path) {