From 34a4e22e97bd39de334388567e7a5cf063445238 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 29 Apr 2023 04:30:32 +0900 Subject: [PATCH 1/4] core/path: align table --- src/core/path.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/path.c b/src/core/path.c index 3881c77a86..9f6a246ab0 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -26,10 +26,10 @@ #include "unit.h" static const UnitActiveState state_translation_table[_PATH_STATE_MAX] = { - [PATH_DEAD] = UNIT_INACTIVE, + [PATH_DEAD] = UNIT_INACTIVE, [PATH_WAITING] = UNIT_ACTIVE, [PATH_RUNNING] = UNIT_ACTIVE, - [PATH_FAILED] = UNIT_FAILED, + [PATH_FAILED] = UNIT_FAILED, }; static int path_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata); From bc6377762c210d1bdd7fd2465930731d87dda576 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 29 Apr 2023 04:31:53 +0900 Subject: [PATCH 2/4] core/path: do not enqueue new job in .trigger_notify callback Otherwise, 1. X.path triggered X.service, and the service has waiting start job, 2. systemctl stop X.service 3. the waiting start job is cancelled to install new stop job, 4. path_trigger_notify() is called, and may reinstall new start job, 5. the stop job cannot be installed, and triggeres assertion. So, instead, let's add a defer event source, then enqueue the new start job after the stop (or any other type) job finished. Fixes https://github.com/systemd/systemd/issues/24577#issuecomment-1522628906. --- src/core/path.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---- src/core/path.h | 2 ++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/core/path.c b/src/core/path.c index 9f6a246ab0..c95663c3aa 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -10,6 +10,7 @@ #include "dbus-path.h" #include "dbus-unit.h" #include "escape.h" +#include "event-util.h" #include "fd-util.h" #include "glob-util.h" #include "inotify-util.h" @@ -300,6 +301,7 @@ static void path_done(Unit *u) { assert(p); + p->trigger_notify_event_source = sd_event_source_disable_unref(p->trigger_notify_event_source); path_free_specs(p); } @@ -575,6 +577,9 @@ static void path_enter_waiting(Path *p, bool initial, bool from_trigger_notify) Unit *trigger; int r; + if (p->trigger_notify_event_source) + (void) event_source_disable(p->trigger_notify_event_source); + /* If the triggered unit is already running, so are we */ trigger = UNIT_TRIGGER(UNIT(p)); if (trigger && !UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(trigger))) { @@ -799,8 +804,28 @@ fail: return 0; } -static void path_trigger_notify(Unit *u, Unit *other) { +static void path_trigger_notify_impl(Unit *u, Unit *other, bool on_defer); + +static int path_trigger_notify_on_defer(sd_event_source *s, void *userdata) { + Path *p = ASSERT_PTR(userdata); + Unit *trigger; + + assert(s); + + trigger = UNIT_TRIGGER(UNIT(p)); + if (!trigger) { + log_unit_error(UNIT(p), "Unit to trigger vanished."); + path_enter_dead(p, PATH_FAILURE_RESOURCES); + return 0; + } + + path_trigger_notify_impl(UNIT(p), trigger, /* on_defer = */ true); + return 0; +} + +static void path_trigger_notify_impl(Unit *u, Unit *other, bool on_defer) { Path *p = PATH(u); + int r; assert(u); assert(other); @@ -826,13 +851,46 @@ static void path_trigger_notify(Unit *u, Unit *other) { if (p->state == PATH_RUNNING && UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) { - log_unit_debug(UNIT(p), "Got notified about unit deactivation."); - path_enter_waiting(p, false, true); + if (!on_defer) + log_unit_debug(u, "Got notified about unit deactivation."); } else if (p->state == PATH_WAITING && !UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) { - log_unit_debug(UNIT(p), "Got notified about unit activation."); - path_enter_waiting(p, false, true); + if (!on_defer) + log_unit_debug(u, "Got notified about unit activation."); + } else + return; + + if (on_defer) { + path_enter_waiting(p, /* initial = */ false, /* from_trigger_notify = */ true); + return; } + + /* Do not call path_enter_waiting() directly from path_trigger_notify(), as this may be called by + * job_install() -> job_finish_and_invalidate() -> unit_trigger_notify(), and path_enter_waiting() + * may install another job and will trigger assertion in job_install(). + * https://github.com/systemd/systemd/issues/24577#issuecomment-1522628906 + * Hence, first setup defer event source here, and call path_enter_waiting() slightly later. */ + if (p->trigger_notify_event_source) { + r = sd_event_source_set_enabled(p->trigger_notify_event_source, SD_EVENT_ONESHOT); + if (r < 0) { + log_unit_warning_errno(u, r, "Failed to enable event source for triggering notify: %m"); + path_enter_dead(p, PATH_FAILURE_RESOURCES); + return; + } + } else { + r = sd_event_add_defer(u->manager->event, &p->trigger_notify_event_source, path_trigger_notify_on_defer, p); + if (r < 0) { + log_unit_warning_errno(u, r, "Failed to allocate event source for triggering notify: %m"); + path_enter_dead(p, PATH_FAILURE_RESOURCES); + return; + } + + (void) sd_event_source_set_description(p->trigger_notify_event_source, "path-trigger-notify"); + } +} + +static void path_trigger_notify(Unit *u, Unit *other) { + path_trigger_notify_impl(u, other, /* on_defer = */ false); } static void path_reset_failed(Unit *u) { diff --git a/src/core/path.h b/src/core/path.h index c76103cc12..cb5b662911 100644 --- a/src/core/path.h +++ b/src/core/path.h @@ -65,6 +65,8 @@ struct Path { PathResult result; RateLimit trigger_limit; + + sd_event_source *trigger_notify_event_source; }; struct ActivationDetailsPath { From 2e701a7946b492cc5f1e56307c4e94110b4c650c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 29 Apr 2023 09:10:11 +0900 Subject: [PATCH 3/4] test: create temporary units under /run --- test/units/testsuite-33.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/units/testsuite-33.sh b/test/units/testsuite-33.sh index c9bd66e268..d48bef73b4 100755 --- a/test/units/testsuite-33.sh +++ b/test/units/testsuite-33.sh @@ -5,7 +5,7 @@ set -eux set -o pipefail -cat >/etc/systemd/system/test-service.service </run/systemd/system/test-service.service </etc/systemd/system/test-service.service </run/systemd/system/test-service.service </etc/systemd/system/tmp-hoge.mount </run/systemd/system/tmp-hoge.mount </etc/systemd/system/test-service.socket </run/systemd/system/test-service.socket < Date: Sat, 29 Apr 2023 04:39:46 +0900 Subject: [PATCH 4/4] test: add tests for "systemctl stop" vs triggering by path unit --- .../test63-issue-24577-dep.service | 4 ++ .../test63-issue-24577.path | 3 ++ .../test63-issue-24577.service | 8 ++++ test/units/testsuite-63.sh | 39 +++++++++++++++++++ 4 files changed, 54 insertions(+) create mode 100644 test/testsuite-63.units/test63-issue-24577-dep.service create mode 100644 test/testsuite-63.units/test63-issue-24577.path create mode 100644 test/testsuite-63.units/test63-issue-24577.service diff --git a/test/testsuite-63.units/test63-issue-24577-dep.service b/test/testsuite-63.units/test63-issue-24577-dep.service new file mode 100644 index 0000000000..e332ea474e --- /dev/null +++ b/test/testsuite-63.units/test63-issue-24577-dep.service @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Service] +Type=oneshot +ExecStart=bash -c 'sleep infinity' diff --git a/test/testsuite-63.units/test63-issue-24577.path b/test/testsuite-63.units/test63-issue-24577.path new file mode 100644 index 0000000000..80ba1dbe16 --- /dev/null +++ b/test/testsuite-63.units/test63-issue-24577.path @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Path] +PathExists=/tmp/hoge diff --git a/test/testsuite-63.units/test63-issue-24577.service b/test/testsuite-63.units/test63-issue-24577.service new file mode 100644 index 0000000000..568518bbf6 --- /dev/null +++ b/test/testsuite-63.units/test63-issue-24577.service @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Unit] +Requires=test63-issue-24577-dep.service +After=test63-issue-24577-dep.service + +[Service] +Type=oneshot +ExecStart=bash -c 'sleep infinity' diff --git a/test/units/testsuite-63.sh b/test/units/testsuite-63.sh index 7ee7fc1513..591e6d3104 100755 --- a/test/units/testsuite-63.sh +++ b/test/units/testsuite-63.sh @@ -3,6 +3,9 @@ set -ex set -o pipefail +# shellcheck source=test/units/assert.sh +. "$(dirname "$0")"/assert.sh + systemctl log-level debug # Test that a path unit continuously triggering a service that fails condition checks eventually fails with @@ -41,6 +44,42 @@ systemctl stop test63-glob.path test63-glob.service test "$(busctl --json=short get-property org.freedesktop.systemd1 /org/freedesktop/systemd1/unit/test63_2dglob_2eservice org.freedesktop.systemd1.Unit ActivationDetails)" = '{"type":"a(ss)","data":[]}' +# tests for issue https://github.com/systemd/systemd/issues/24577#issuecomment-1522628906 +rm -f /tmp/hoge +systemctl start test63-issue-24577.path +systemctl status -n 0 test63-issue-24577.path +systemctl status -n 0 test63-issue-24577.service || : +systemctl list-jobs +output=$(systemctl list-jobs --no-legend) +assert_not_in "test63-issue-24577.service" "$output" +assert_not_in "test63-issue-24577-dep.service" "$output" + +touch /tmp/hoge +systemctl status -n 0 test63-issue-24577.path +systemctl status -n 0 test63-issue-24577.service || : +systemctl list-jobs +output=$(systemctl list-jobs --no-legend) +assert_in "test63-issue-24577.service" "$output" +assert_in "test63-issue-24577-dep.service" "$output" + +# even if the service is stopped, it will be soon retriggered. +systemctl stop test63-issue-24577.service +systemctl status -n 0 test63-issue-24577.path +systemctl status -n 0 test63-issue-24577.service || : +systemctl list-jobs +output=$(systemctl list-jobs --no-legend) +assert_in "test63-issue-24577.service" "$output" +assert_in "test63-issue-24577-dep.service" "$output" + +rm -f /tmp/hoge +systemctl stop test63-issue-24577.service +systemctl status -n 0 test63-issue-24577.path +systemctl status -n 0 test63-issue-24577.service || : +systemctl list-jobs +output=$(systemctl list-jobs --no-legend) +assert_not_in "test63-issue-24577.service" "$output" +assert_in "test63-issue-24577-dep.service" "$output" + systemctl log-level info echo OK >/testok