Merge pull request #20001 from keszybz/test-path-simplify-less

Do not call path_simplify() when not needed

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

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}"

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) {

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) {

src/test/meson.build

@@ -370,6 +370,8 @@ tests += [
          [],
          [threads]],
 
+        [['src/test/test-hash-funcs.c']],
+
         [['src/test/test-bitmap.c']],
 
         [['src/test/test-xml.c']],

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();
+}

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);