diff --git a/src/core/unit.c b/src/core/unit.c index b7cca3e287..641993b836 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); @@ -938,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( @@ -1045,11 +1032,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) { - - int r; +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. */ assert(u); assert(other); @@ -1057,18 +1043,29 @@ 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_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)); + } + 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); 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) { @@ -1078,7 +1075,12 @@ static void unit_merge_dependencies( 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; } @@ -1097,41 +1099,21 @@ static void unit_merge_dependencies( 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) { - 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)); - - 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); - TAKE_PTR(other_deps); - - if (hashmap_remove(other_deps, u)) - unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + 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); @@ -1177,11 +1159,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); @@ -1189,6 +1166,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; @@ -3067,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; @@ -3086,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; } diff --git a/test/units/testsuite-26.sh b/test/units/testsuite-26.sh index 0fe0d9b0d9..2e0350aabe 100755 --- a/test/units/testsuite-26.sh +++ b/test/units/testsuite-26.sh @@ -4,10 +4,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 -fvr "/usr/lib/systemd/system/$UNIT_NAME" "/etc/systemd/system/$UNIT_NAME.d" "+4" fi + + rm -f /etc/init.d/issue-24990 + return 0 } # shellcheck source=test/units/assert.sh @@ -307,6 +315,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