From ca1999a24c88b2460240aaff8095c2db7491f77c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 15 Nov 2022 12:54:11 +0900 Subject: [PATCH 1/7] core/unit: drop doubled empty line --- src/core/unit.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/unit.c b/src/core/unit.c index 29b07a6e7a..36e3afd7fb 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -721,7 +721,6 @@ Unit* unit_free(Unit *u) { if (u->on_console) manager_unref_console(u->manager); - fdset_free(u->initial_socket_bind_link_fds); #if BPF_FRAMEWORK bpf_link_free(u->ipv4_socket_bind_link); From c8b3b524134539846917269ddd644ee93a35623f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 16 Nov 2022 03:08:22 +0900 Subject: [PATCH 2/7] core/unit: drop dependency to the unit being merged Fixes a bug in 15ed3c3a188cf7fa5a60ae508fc7a3ed048d2220. Fixes #24990. Also, hopefully fixes #24577. --- src/core/unit.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 36e3afd7fb..1a580157af 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1044,10 +1044,10 @@ static int unit_add_dependency_hashmap( return unit_per_dependency_type_hashmap_update(per_type, other, origin_mask, destination_mask); } -static void unit_merge_dependencies( - Unit *u, - Unit *other) { - +static void unit_merge_dependencies(Unit *u, Unit *other) { + Hashmap *deps; + void *dt; /* Actually of type UnitDependency, except that we don't bother casting it here, + * since the hashmaps all want it as void pointer. */ int r; assert(u); @@ -1056,12 +1056,19 @@ static void unit_merge_dependencies( if (u == other) return; + /* First, remove dependency to other. */ + HASHMAP_FOREACH_KEY(deps, dt, u->dependencies) { + if (hashmap_remove(deps, other)) + unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + + if (hashmap_isempty(deps)) + hashmap_free(hashmap_remove(u->dependencies, dt)); + } + for (;;) { _cleanup_(hashmap_freep) Hashmap *other_deps = NULL; UnitDependencyInfo di_back; Unit *back; - void *dt; /* Actually of type UnitDependency, except that we don't bother casting it here, - * since the hashmaps all want it as void pointer. */ /* Let's focus on one dependency type at a time, that 'other' has defined. */ other_deps = hashmap_steal_first_key_and_value(other->dependencies, &dt); @@ -1103,8 +1110,6 @@ static void unit_merge_dependencies( * them per type wholesale. */ r = hashmap_put(u->dependencies, dt, other_deps); if (r == -EEXIST) { - Hashmap *deps; - /* The target unit already has dependencies of this type, let's then merge this individually. */ assert_se(deps = hashmap_get(u->dependencies, dt)); From 53e0e6ef0eea396bb432cbfc1f2f6ea1272ff1f1 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 15 Nov 2022 23:08:35 +0900 Subject: [PATCH 3/7] core/unit: fix logic of dropping self-referencing dependencies Fixes a bug in 15ed3c3a188cf7fa5a60ae508fc7a3ed048d2220. --- src/core/unit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/unit.c b/src/core/unit.c index 1a580157af..a9052428e4 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1131,10 +1131,11 @@ static void unit_merge_dependencies(Unit *u, Unit *other) { } } else { assert_se(r >= 0); - TAKE_PTR(other_deps); if (hashmap_remove(other_deps, u)) unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + + TAKE_PTR(other_deps); } } From 4b7918a65cc2571a2b3fc166229e1b8db463e217 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 17 Nov 2022 12:46:45 +0900 Subject: [PATCH 4/7] core/unit: merge two loops into one No functional change, just refactoring. --- src/core/unit.c | 47 +++++++++++++++-------------------------------- 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index a9052428e4..0d52e4bf1a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1048,7 +1048,6 @@ static void unit_merge_dependencies(Unit *u, Unit *other) { Hashmap *deps; void *dt; /* Actually of type UnitDependency, except that we don't bother casting it here, * since the hashmaps all want it as void pointer. */ - int r; assert(u); assert(other); @@ -1075,6 +1074,8 @@ static void unit_merge_dependencies(Unit *u, Unit *other) { if (!other_deps) break; /* done! */ + deps = hashmap_get(u->dependencies, dt); + /* Now iterate through all dependencies of this dependency type, of 'other'. We refer to the * referenced units as 'back'. */ HASHMAP_FOREACH_KEY(di_back.data, back, other_deps) { @@ -1085,6 +1086,7 @@ static void unit_merge_dependencies(Unit *u, Unit *other) { /* This is a dependency pointing back to the unit we want to merge with? * Suppress it (but warn) */ unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + hashmap_remove(other_deps, back); continue; } @@ -1103,40 +1105,21 @@ static void unit_merge_dependencies(Unit *u, Unit *other) { di_move.origin_mask, di_move.destination_mask) >= 0); } - } - /* Now all references towards 'other' of the current type 'dt' are corrected to point to - * 'u'. Lets's now move the deps of type 'dt' from 'other' to 'u'. First, let's try to move - * them per type wholesale. */ - r = hashmap_put(u->dependencies, dt, other_deps); - if (r == -EEXIST) { /* The target unit already has dependencies of this type, let's then merge this individually. */ - - assert_se(deps = hashmap_get(u->dependencies, dt)); - - for (;;) { - UnitDependencyInfo di_move; - - /* Get first dep */ - di_move.data = hashmap_steal_first_key_and_value(other_deps, (void**) &back); - if (!di_move.data) - break; /* done */ - if (back == u) { - /* Would point back to us, ignore */ - unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); - continue; - } - - assert_se(unit_per_dependency_type_hashmap_update(deps, back, di_move.origin_mask, di_move.destination_mask) >= 0); - } - } else { - assert_se(r >= 0); - - if (hashmap_remove(other_deps, u)) - unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); - - TAKE_PTR(other_deps); + if (deps) + assert_se(unit_per_dependency_type_hashmap_update( + deps, + back, + di_back.origin_mask, + di_back.destination_mask) >= 0); } + + /* Now all references towards 'other' of the current type 'dt' are corrected to point to 'u'. + * Lets's now move the deps of type 'dt' from 'other' to 'u'. If the unit does not have + * dependencies of this type, let's move them per type wholesale. */ + if (!deps) + assert_se(hashmap_put(u->dependencies, dt, TAKE_PTR(other_deps)) >= 0); } other->dependencies = hashmap_free(other->dependencies); From 5f882cc3ab32636d9242effb2cefad20d92d2ec2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 15 Nov 2022 21:52:10 +0900 Subject: [PATCH 5/7] test: add test case for sysv-generator and invalid dependency --- test/units/testsuite-26.sh | 116 ++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 2 deletions(-) diff --git a/test/units/testsuite-26.sh b/test/units/testsuite-26.sh index 7c7a12b1ae..a8e7a5abaa 100755 --- a/test/units/testsuite-26.sh +++ b/test/units/testsuite-26.sh @@ -3,10 +3,18 @@ set -eux set -o pipefail +# shellcheck source=test/units/assert.sh +. "$(dirname "$0")"/assert.sh + +: >/failed + at_exit() { if [[ -v UNIT_NAME && -e "/usr/lib/systemd/system/$UNIT_NAME" ]]; then rm -fv "/usr/lib/systemd/system/$UNIT_NAME" fi + + rm -f /etc/init.d/issue-24990 + return 0 } trap at_exit EXIT @@ -284,6 +292,110 @@ systemctl unset-environment IMPORT_THIS IMPORT_THIS_TOO (! systemctl show-environment | grep "^IMPORT_THIS=") (! systemctl show-environment | grep "^IMPORT_THIS_TOO=") -echo OK >/testok +# test for sysv-generator (issue #24990) +if [[ -x /usr/lib/systemd/system-generators/systemd-sysv-generator ]]; then -exit 0 + # invalid dependency + cat >/etc/init.d/issue-24990 <<\EOF +#!/bin/bash + +### BEGIN INIT INFO +# Provides:test1 test2 +# Required-Start:test1 $remote_fs $network +# Required-Stop:test1 $remote_fs $network +# Description:Test +# Short-Description: Test +### END INIT INFO + +case "$1" in + start) + echo "Starting issue-24990.service" + sleep 1000 & + ;; + stop) + echo "Stopping issue-24990.service" + sleep 10 & + ;; + *) + echo "Usage: service test {start|stop|restart|status}" + ;; +esac +EOF + + chmod +x /etc/init.d/issue-24990 + systemctl daemon-reload + [[ -L /run/systemd/generator.late/test1.service ]] + [[ -L /run/systemd/generator.late/test2.service ]] + assert_eq "$(readlink -f /run/systemd/generator.late/test1.service)" "/run/systemd/generator.late/issue-24990.service" + assert_eq "$(readlink -f /run/systemd/generator.late/test2.service)" "/run/systemd/generator.late/issue-24990.service" + output=$(systemctl cat issue-24990) + assert_in "SourcePath=/etc/init.d/issue-24990" "$output" + assert_in "Description=LSB: Test" "$output" + assert_in "After=test1.service" "$output" + assert_in "After=remote-fs.target" "$output" + assert_in "After=network-online.target" "$output" + assert_in "Wants=network-online.target" "$output" + assert_in "ExecStart=/etc/init.d/issue-24990 start" "$output" + assert_in "ExecStop=/etc/init.d/issue-24990 stop" "$output" + systemctl status issue-24990 || : + systemctl show issue-24990 + assert_not_in "issue-24990.service" "$(systemctl show --property=After --value)" + assert_not_in "issue-24990.service" "$(systemctl show --property=Before --value)" + + if ! systemctl is-active network-online.target; then + systemctl start network-online.target + fi + + systemctl restart issue-24990 + systemctl stop issue-24990 + + # valid dependency + cat >/etc/init.d/issue-24990 <<\EOF +#!/bin/bash + +### BEGIN INIT INFO +# Provides:test1 test2 +# Required-Start:$remote_fs +# Required-Stop:$remote_fs +# Description:Test +# Short-Description: Test +### END INIT INFO + +case "$1" in + start) + echo "Starting issue-24990.service" + sleep 1000 & + ;; + stop) + echo "Stopping issue-24990.service" + sleep 10 & + ;; + *) + echo "Usage: service test {start|stop|restart|status}" + ;; +esac +EOF + + chmod +x /etc/init.d/issue-24990 + systemctl daemon-reload + [[ -L /run/systemd/generator.late/test1.service ]] + [[ -L /run/systemd/generator.late/test2.service ]] + assert_eq "$(readlink -f /run/systemd/generator.late/test1.service)" "/run/systemd/generator.late/issue-24990.service" + assert_eq "$(readlink -f /run/systemd/generator.late/test2.service)" "/run/systemd/generator.late/issue-24990.service" + output=$(systemctl cat issue-24990) + assert_in "SourcePath=/etc/init.d/issue-24990" "$output" + assert_in "Description=LSB: Test" "$output" + assert_in "After=remote-fs.target" "$output" + assert_in "ExecStart=/etc/init.d/issue-24990 start" "$output" + assert_in "ExecStop=/etc/init.d/issue-24990 stop" "$output" + systemctl status issue-24990 || : + systemctl show issue-24990 + assert_not_in "issue-24990.service" "$(systemctl show --property=After --value)" + assert_not_in "issue-24990.service" "$(systemctl show --property=Before --value)" + + systemctl restart issue-24990 + systemctl stop issue-24990 +fi + +touch /testok +rm /failed From 1d0c81a05b1605a5fc3db44d5a157a1d6876eda9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 16 Nov 2022 03:18:30 +0900 Subject: [PATCH 6/7] core/unit: merge unit names after merging deps Before: systemd[1]: issue-24990.service: Dependency Before=n/a dropped, merged into issue-24990.service After: systemd[1]: issue-24990.service: Dependency Before=test1.service dropped, merged into issue-24990.service --- src/core/unit.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 0d52e4bf1a..ea09416be5 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1165,11 +1165,6 @@ int unit_merge(Unit *u, Unit *other) { if (r < 0) return r; - /* Merge names */ - r = unit_merge_names(u, other); - if (r < 0) - return r; - /* Redirect all references */ while (other->refs_by_target) unit_ref_set(other->refs_by_target, other->refs_by_target->source, u); @@ -1177,6 +1172,11 @@ int unit_merge(Unit *u, Unit *other) { /* Merge dependencies */ unit_merge_dependencies(u, other); + /* Merge names. It is better to do that after merging deps, otherwise the log message contains n/a. */ + r = unit_merge_names(u, other); + if (r < 0) + return r; + other->load_state = UNIT_MERGED; other->merged_into = u; From ed9911630e4bca844381d7caeb850dad9a9fa122 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 15 Nov 2022 22:59:01 +0900 Subject: [PATCH 7/7] core/unit: fix log message As you can see in the below, the dropped dependency Before=issue-24990.service is not logged, but the dependency Before=test1.service which is not owned by the units generated by the TEST-26 is logged. Before: systemd[1]: issue-24990.service: Dependency After=test1.service dropped, merged into issue-24990.service systemd[1]: issue-24990.service: Dependency Before=test1.service dropped, merged into issue-24990.service After: systemd[1]: issue-24990.service: Dependency After=test1.service is dropped, as test1.service is merged into issue-24990.service. systemd[1]: issue-24990.service: Dependency Before=issue-24990.service in test1.service is dropped, as test1.service is merged into issue-24990.service. --- src/core/unit.c | 49 ++++++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index ea09416be5..988ba8e34a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -937,29 +937,17 @@ static int unit_reserve_dependencies(Unit *u, Unit *other) { return 0; } -static void unit_maybe_warn_about_dependency( - Unit *u, - const char *other_id, - UnitDependency dependency) { - - assert(u); - +static bool unit_should_warn_about_dependency(UnitDependency dependency) { /* Only warn about some unit types */ - if (!IN_SET(dependency, - UNIT_CONFLICTS, - UNIT_CONFLICTED_BY, - UNIT_BEFORE, - UNIT_AFTER, - UNIT_ON_SUCCESS, - UNIT_ON_FAILURE, - UNIT_TRIGGERS, - UNIT_TRIGGERED_BY)) - return; - - if (streq_ptr(u->id, other_id)) - log_unit_warning(u, "Dependency %s=%s dropped", unit_dependency_to_string(dependency), u->id); - else - log_unit_warning(u, "Dependency %s=%s dropped, merged into %s", unit_dependency_to_string(dependency), strna(other_id), u->id); + return IN_SET(dependency, + UNIT_CONFLICTS, + UNIT_CONFLICTED_BY, + UNIT_BEFORE, + UNIT_AFTER, + UNIT_ON_SUCCESS, + UNIT_ON_FAILURE, + UNIT_TRIGGERS, + UNIT_TRIGGERED_BY); } static int unit_per_dependency_type_hashmap_update( @@ -1057,8 +1045,10 @@ static void unit_merge_dependencies(Unit *u, Unit *other) { /* First, remove dependency to other. */ HASHMAP_FOREACH_KEY(deps, dt, u->dependencies) { - if (hashmap_remove(deps, other)) - unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + if (hashmap_remove(deps, other) && unit_should_warn_about_dependency(UNIT_DEPENDENCY_FROM_PTR(dt))) + log_unit_warning(u, "Dependency %s=%s is dropped, as %s is merged into %s.", + unit_dependency_to_string(UNIT_DEPENDENCY_FROM_PTR(dt)), + other->id, other->id, u->id); if (hashmap_isempty(deps)) hashmap_free(hashmap_remove(u->dependencies, dt)); @@ -1085,7 +1075,11 @@ static void unit_merge_dependencies(Unit *u, Unit *other) { if (back == u) { /* This is a dependency pointing back to the unit we want to merge with? * Suppress it (but warn) */ - unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + if (unit_should_warn_about_dependency(UNIT_DEPENDENCY_FROM_PTR(dt))) + log_unit_warning(u, "Dependency %s=%s in %s is dropped, as %s is merged into %s.", + unit_dependency_to_string(UNIT_DEPENDENCY_FROM_PTR(dt)), + u->id, other->id, other->id, u->id); + hashmap_remove(other_deps, back); continue; } @@ -3055,7 +3049,6 @@ int unit_add_dependency( [UNIT_IN_SLICE] = UNIT_SLICE_OF, [UNIT_SLICE_OF] = UNIT_IN_SLICE, }; - Unit *original_u = u, *original_other = other; UnitDependencyAtom a; int r; @@ -3074,7 +3067,9 @@ int unit_add_dependency( /* We won't allow dependencies on ourselves. We will not consider them an error however. */ if (u == other) { - unit_maybe_warn_about_dependency(original_u, original_other->id, d); + if (unit_should_warn_about_dependency(d)) + log_unit_warning(u, "Dependency %s=%s is dropped.", + unit_dependency_to_string(d), u->id); return 0; }