Skip to content

Commit 86e24d6

Browse files
poetteringpoettering
authored
Merge pull request #20001 from keszybz/test-path-simplify-less
Do not call path_simplify() when not needed
2 parents 6abd991 + ac19bdd commit 86e24d6

7 files changed

Lines changed: 117 additions & 24 deletions

File tree

‎TODO‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -885,6 +885,10 @@ Features:
885885

886886
* fstab-generator: default to tmpfs-as-root if only usr= is specified on the kernel cmdline
887887

888+
* initrd-parse-etc.service: can we skip daemon-reload if /sysroot/etc/fstab is missing?
889+
Note that we start initrd-fs.target and initrd-cleanup.target there, so a straightforward
890+
ConditionPathExists= is not enough.
891+
888892
* docs: bring http://www.freedesktop.org/wiki/Software/systemd/MyServiceCantGetRealtime up to date
889893

890894
* add a job mode that will fail if a transaction would mean stopping

‎man/50-xdg-data-dirs.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@ XDG_DATA_DIRS="${XDG_DATA_DIRS:-/usr/local/share/:/usr/share}"
55

66
# add a directory if it exists
77
if [[ -d /opt/foo/share ]]; then
8-
XDG_DATA_DIRS=/opt/foo/share:${XDG_DATA_DIRS}
8+
XDG_DATA_DIRS="/opt/foo/share:${XDG_DATA_DIRS}"
99
fi
1010

1111
# write our output
12-
echo XDG_DATA_DIRS=$XDG_DATA_DIRS
12+
echo "XDG_DATA_DIRS=${XDG_DATA_DIRS}"

‎src/core/manager.c‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4511,16 +4511,14 @@ void manager_status_printf(Manager *m, StatusType type, const char *status, cons
45114511
va_end(ap);
45124512
}
45134513

4514-
Set *manager_get_units_requiring_mounts_for(Manager *m, const char *path) {
4515-
char p[strlen(path)+1];
4516-
4514+
Set* manager_get_units_requiring_mounts_for(Manager *m, const char *path) {
45174515
assert(m);
45184516
assert(path);
45194517

4520-
strcpy(p, path);
4521-
path_simplify(p);
4518+
if (path_equal(path, "/"))
4519+
path = "";
45224520

4523-
return hashmap_get(m->units_requiring_mounts_for, streq(p, "/") ? "" : p);
4521+
return hashmap_get(m->units_requiring_mounts_for, path);
45244522
}
45254523

45264524
int manager_update_failed_units(Manager *m, Unit *u, bool failed) {

‎src/core/unit.c‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4562,45 +4562,43 @@ int unit_kill_context(
45624562
}
45634563

45644564
int unit_require_mounts_for(Unit *u, const char *path, UnitDependencyMask mask) {
4565-
_cleanup_free_ char *p = NULL;
4566-
UnitDependencyInfo di;
45674565
int r;
45684566

45694567
assert(u);
45704568
assert(path);
45714569

4572-
/* Registers a unit for requiring a certain path and all its prefixes. We keep a hashtable of these paths in
4573-
* the unit (from the path to the UnitDependencyInfo structure indicating how to the dependency came to
4574-
* be). However, we build a prefix table for all possible prefixes so that new appearing mount units can easily
4575-
* determine which units to make themselves a dependency of. */
4570+
/* Registers a unit for requiring a certain path and all its prefixes. We keep a hashtable of these
4571+
* paths in the unit (from the path to the UnitDependencyInfo structure indicating how to the
4572+
* dependency came to be). However, we build a prefix table for all possible prefixes so that new
4573+
* appearing mount units can easily determine which units to make themselves a dependency of. */
45764574

45774575
if (!path_is_absolute(path))
45784576
return -EINVAL;
45794577

4580-
r = hashmap_ensure_allocated(&u->requires_mounts_for, &path_hash_ops);
4581-
if (r < 0)
4582-
return r;
4578+
if (hashmap_contains(u->requires_mounts_for, path)) /* Exit quickly if the path is already covered. */
4579+
return 0;
45834580

4584-
p = strdup(path);
4581+
_cleanup_free_ char *p = strdup(path);
45854582
if (!p)
45864583
return -ENOMEM;
45874584

4585+
/* Use the canonical form of the path as the stored key. We call path_is_normalized()
4586+
* only after simplification, since path_is_normalized() rejects paths with '.'.
4587+
* path_is_normalized() also verifies that the path fits in PATH_MAX. */
45884588
path = path_simplify(p);
45894589

45904590
if (!path_is_normalized(path))
45914591
return -EPERM;
45924592

4593-
if (hashmap_contains(u->requires_mounts_for, path))
4594-
return 0;
4595-
4596-
di = (UnitDependencyInfo) {
4593+
UnitDependencyInfo di = {
45974594
.origin_mask = mask
45984595
};
45994596

4600-
r = hashmap_put(u->requires_mounts_for, path, di.data);
4597+
r = hashmap_ensure_put(&u->requires_mounts_for, &path_hash_ops, p, di.data);
46014598
if (r < 0)
46024599
return r;
4603-
p = NULL;
4600+
assert(r > 0);
4601+
TAKE_PTR(p); /* path remains a valid pointer to the string stored in the hashmap */
46044602

46054603
char prefix[strlen(path) + 1];
46064604
PATH_FOREACH_PREFIX_MORE(prefix, path) {

‎src/test/meson.build‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,8 @@ tests += [
370370
[],
371371
[threads]],
372372

373+
[['src/test/test-hash-funcs.c']],
374+
373375
[['src/test/test-bitmap.c']],
374376

375377
[['src/test/test-xml.c']],

‎src/test/test-hash-funcs.c‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/* SPDX-License-Identifier: LGPL-2.1-or-later */
2+
3+
#include "tests.h"
4+
#include "hash-funcs.h"
5+
#include "set.h"
6+
7+
static void test_path_hash_set(void) {
8+
/* The goal is to make sure that non-simplified path are hashed as expected,
9+
* and that we don't need to simplify them beforehand. */
10+
11+
log_info("/* %s */", __func__);
12+
13+
/* No freeing of keys, we operate on static strings here… */
14+
_cleanup_set_free_ Set *set = NULL;
15+
16+
assert_se(set_isempty(set));
17+
assert_se(set_ensure_put(&set, &path_hash_ops, "foo") == 1);
18+
assert_se(set_ensure_put(&set, &path_hash_ops, "foo") == 0);
19+
assert_se(set_ensure_put(&set, &path_hash_ops, "bar") == 1);
20+
assert_se(set_ensure_put(&set, &path_hash_ops, "bar") == 0);
21+
assert_se(set_ensure_put(&set, &path_hash_ops, "/foo") == 1);
22+
assert_se(set_ensure_put(&set, &path_hash_ops, "/bar") == 1);
23+
assert_se(set_ensure_put(&set, &path_hash_ops, "/foo/.") == 0);
24+
assert_se(set_ensure_put(&set, &path_hash_ops, "/./bar/./.") == 0);
25+
26+
assert_se(set_contains(set, "foo"));
27+
assert_se(set_contains(set, "bar"));
28+
assert_se(set_contains(set, "./foo"));
29+
assert_se(set_contains(set, "./foo/."));
30+
assert_se(set_contains(set, "./bar"));
31+
assert_se(set_contains(set, "./bar/."));
32+
assert_se(set_contains(set, "/foo"));
33+
assert_se(set_contains(set, "/bar"));
34+
assert_se(set_contains(set, "//./foo"));
35+
assert_se(set_contains(set, "///./foo/."));
36+
assert_se(set_contains(set, "////./bar"));
37+
assert_se(set_contains(set, "/////./bar/."));
38+
39+
assert_se(set_contains(set, "foo/"));
40+
assert_se(set_contains(set, "bar/"));
41+
assert_se(set_contains(set, "./foo/"));
42+
assert_se(set_contains(set, "./foo/./"));
43+
assert_se(set_contains(set, "./bar/"));
44+
assert_se(set_contains(set, "./bar/./"));
45+
assert_se(set_contains(set, "/foo/"));
46+
assert_se(set_contains(set, "/bar/"));
47+
assert_se(set_contains(set, "//./foo/"));
48+
assert_se(set_contains(set, "///./foo/./"));
49+
assert_se(set_contains(set, "////./bar/"));
50+
assert_se(set_contains(set, "/////./bar/./"));
51+
52+
assert_se(!set_contains(set, "foo."));
53+
assert_se(!set_contains(set, ".bar"));
54+
assert_se(!set_contains(set, "./foo."));
55+
assert_se(!set_contains(set, "./.foo/."));
56+
assert_se(!set_contains(set, "../bar"));
57+
assert_se(!set_contains(set, "./bar/.."));
58+
assert_se(!set_contains(set, "./foo.."));
59+
assert_se(!set_contains(set, "/..bar"));
60+
assert_se(!set_contains(set, "//../foo"));
61+
assert_se(!set_contains(set, "///../foo/."));
62+
assert_se(!set_contains(set, "////../bar"));
63+
assert_se(!set_contains(set, "/////../bar/."));
64+
65+
assert_se(!set_contains(set, "foo./"));
66+
assert_se(!set_contains(set, ".bar/"));
67+
assert_se(!set_contains(set, "./foo./"));
68+
assert_se(!set_contains(set, "./.foo/./"));
69+
assert_se(!set_contains(set, "../bar/"));
70+
assert_se(!set_contains(set, "./bar/../"));
71+
assert_se(!set_contains(set, "./foo../"));
72+
assert_se(!set_contains(set, "/..bar/"));
73+
assert_se(!set_contains(set, "//../foo/"));
74+
assert_se(!set_contains(set, "///../foo/./"));
75+
assert_se(!set_contains(set, "////../bar/"));
76+
assert_se(!set_contains(set, "/////../bar/./"));
77+
}
78+
79+
int main(int argc, char **argv) {
80+
test_setup_logging(LOG_INFO);
81+
82+
test_path_hash_set();
83+
}

‎src/test/test-path-util.c‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,8 @@ static void test_path_compare_one(const char *a, const char *b, int expected) {
126126
}
127127

128128
static void test_path_compare(void) {
129+
log_info("/* %s */", __func__);
130+
129131
test_path_compare_one("/goo", "/goo", 0);
130132
test_path_compare_one("/goo", "/goo", 0);
131133
test_path_compare_one("//goo", "/goo", 0);
@@ -138,6 +140,12 @@ static void test_path_compare(void) {
138140
test_path_compare_one("/x", "x/", 1);
139141
test_path_compare_one("x/", "/", -1);
140142
test_path_compare_one("/x/./y", "x/y", 1);
143+
test_path_compare_one("/x/./y", "/x/y", 0);
144+
test_path_compare_one("/x/./././y", "/x/y/././.", 0);
145+
test_path_compare_one("./x/./././y", "./x/y/././.", 0);
146+
test_path_compare_one(".", "./.", 0);
147+
test_path_compare_one(".", "././.", 0);
148+
test_path_compare_one("./..", ".", 1);
141149
test_path_compare_one("x/.y", "x/y", -1);
142150
test_path_compare_one("foo", "/foo", -1);
143151
test_path_compare_one("/foo", "/foo/bar", -1);

0 commit comments

Comments
 (0)