From 708961c7011b7578d3e6e10ca463d15edb62b5c1 Mon Sep 17 00:00:00 2001 From: Michael Chapman Date: Tue, 5 May 2020 13:50:04 +1000 Subject: [PATCH] core/path: recheck path specs when triggered unit changes state As documented in systemd.path(5): When a service unit triggered by a path unit terminates (regardless whether it exited successfully or failed), monitored paths are checked immediately again, and the service accordingly restarted instantly. This commit implements this behaviour for PathExists=, PathExistsGlob=, and DirectoryNotEmpty=. These predicates are essentially "level-triggered": the service should be activated whenever the predicate is true. PathChanged= and PathModified=, on the other hand, are "edge-triggered": the service should only be activated when the predicate *becomes* true. The behaviour has been broken since at least as far back as commit 8fca6944c2 ("path: stop watching path specs once we triggered the target unit"). This commit had systemd stop monitoring inotify whenever the triggered unit was activated. Unfortunately this meant it never updated the ->inotify_triggered flag, so it never rechecked the path specs when the triggered unit deactivated. With this commit, systemd rechecks all paths specs whenever the triggered unit deactivates. If any PathExists=, PathExistsGlob= or DirectoryNotEmpty= predicate passes, the triggered unit is reactivated. If the target unit is activated by something outside of the path unit, the path unit immediately transitions to a running state. This ensures the path unit stops monitoring inotify in this situation. With this change in place, commit d7cf8c24d4 ("core/path: fix spurious triggering of PathExists= on restart/reload") is no longer necessary. The path unit (and its triggered unit) is now always active whenever the PathExists= predicate passes, so there is no spurious restart when systemd is reloaded or restarted. --- src/core/path.c | 73 ++++---- src/core/path.h | 2 - src/test/test-path.c | 160 +++++++++++++++--- test/test-path/path-changed.service | 3 +- test/test-path/path-directorynotempty.service | 3 +- test/test-path/path-exists.service | 3 +- test/test-path/path-existsglob.service | 3 +- test/test-path/path-makedirectory.service | 3 +- test/test-path/path-modified.service | 3 +- test/test-path/path-mycustomunit.service | 5 +- 10 files changed, 182 insertions(+), 76 deletions(-) diff --git a/src/core/path.c b/src/core/path.c index d216dcb850..a98a0a14e9 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -174,15 +174,13 @@ int path_spec_fd_event(PathSpec *s, uint32_t revents) { return r; } -static bool path_spec_check_good(PathSpec *s, bool initial) { +static bool path_spec_check_good(PathSpec *s, bool initial, bool from_trigger_notify) { bool b, good = false; switch (s->type) { case PATH_EXISTS: - b = access(s->path, F_OK) >= 0; - good = b && !s->previous_exists; - s->previous_exists = b; + good = access(s->path, F_OK) >= 0; break; case PATH_EXISTS_GLOB: @@ -200,7 +198,7 @@ static bool path_spec_check_good(PathSpec *s, bool initial) { case PATH_CHANGED: case PATH_MODIFIED: b = access(s->path, F_OK) >= 0; - good = !initial && b != s->previous_exists; + good = !initial && !from_trigger_notify && b != s->previous_exists; s->previous_exists = b; break; @@ -426,8 +424,7 @@ static void path_set_state(Path *p, PathState state) { old_state = p->state; p->state = state; - if (state != PATH_WAITING && - (state != PATH_RUNNING || p->inotify_triggered)) + if (!IN_SET(state, PATH_WAITING, PATH_RUNNING)) path_unwatch(p); if (state != old_state) @@ -436,7 +433,7 @@ static void path_set_state(Path *p, PathState state) { unit_notify(UNIT(p), state_translation_table[old_state], state_translation_table[state], 0); } -static void path_enter_waiting(Path *p, bool initial, bool recheck); +static void path_enter_waiting(Path *p, bool initial, bool from_trigger_notify); static int path_coldplug(Unit *u) { Path *p = PATH(u); @@ -447,7 +444,7 @@ static int path_coldplug(Unit *u) { if (p->deserialized_state != p->state) { if (IN_SET(p->deserialized_state, PATH_WAITING, PATH_RUNNING)) - path_enter_waiting(p, true, true); + path_enter_waiting(p, true, false); else path_set_state(p, p->deserialized_state); } @@ -487,8 +484,6 @@ static void path_enter_running(Path *p) { if (r < 0) goto fail; - p->inotify_triggered = false; - path_set_state(p, PATH_RUNNING); path_unwatch(p); @@ -499,27 +494,35 @@ fail: path_enter_dead(p, PATH_FAILURE_RESOURCES); } -static bool path_check_good(Path *p, bool initial) { +static bool path_check_good(Path *p, bool initial, bool from_trigger_notify) { PathSpec *s; assert(p); LIST_FOREACH(spec, s, p->specs) - if (path_spec_check_good(s, initial)) + if (path_spec_check_good(s, initial, from_trigger_notify)) return true; return false; } -static void path_enter_waiting(Path *p, bool initial, bool recheck) { +static void path_enter_waiting(Path *p, bool initial, bool from_trigger_notify) { + Unit *trigger; int r; - if (recheck) - if (path_check_good(p, initial)) { - log_unit_debug(UNIT(p), "Got triggered."); - path_enter_running(p); - return; - } + /* 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))) { + path_set_state(p, PATH_RUNNING); + path_unwatch(p); + return; + } + + if (path_check_good(p, initial, from_trigger_notify)) { + log_unit_debug(UNIT(p), "Got triggered."); + path_enter_running(p); + return; + } r = path_watch(p); if (r < 0) @@ -529,12 +532,11 @@ static void path_enter_waiting(Path *p, bool initial, bool recheck) { * might have appeared/been removed by now, so we must * recheck */ - if (recheck) - if (path_check_good(p, false)) { - log_unit_debug(UNIT(p), "Got triggered."); - path_enter_running(p); - return; - } + if (path_check_good(p, false, from_trigger_notify)) { + log_unit_debug(UNIT(p), "Got triggered."); + path_enter_running(p); + return; + } path_set_state(p, PATH_WAITING); return; @@ -580,7 +582,7 @@ static int path_start(Unit *u) { path_mkdir(p); p->result = PATH_SUCCESS; - path_enter_waiting(p, true, true); + path_enter_waiting(p, true, false); return 1; } @@ -727,15 +729,10 @@ static int path_dispatch_io(sd_event_source *source, int fd, uint32_t revents, v if (changed < 0) goto fail; - /* If we are already running, then remember that one event was - * dispatched so that we restart the service only if something - * actually changed on disk */ - p->inotify_triggered = true; - if (changed) path_enter_running(p); else - path_enter_waiting(p, false, true); + path_enter_waiting(p, false, false); return 0; @@ -759,11 +756,11 @@ 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."); - - /* Hmm, so inotify was triggered since the - * last activation, so I guess we need to - * recheck what is going on. */ - path_enter_waiting(p, false, p->inotify_triggered); + path_enter_waiting(p, false, true); + } 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); } } diff --git a/src/core/path.h b/src/core/path.h index 4d4b6236c2..9e2836535a 100644 --- a/src/core/path.h +++ b/src/core/path.h @@ -56,8 +56,6 @@ struct Path { PathState state, deserialized_state; - bool inotify_triggered; - bool make_directory; mode_t directory_mode; diff --git a/src/test/test-path.c b/src/test/test-path.c index 830d5f261b..0b2b4ab554 100644 --- a/src/test/test-path.c +++ b/src/test/test-path.c @@ -61,28 +61,34 @@ static void shutdown_test(Manager *m) { manager_free(m); } -static void check_stop_unlink(Manager *m, Unit *unit, const char *test_path, const char *service_name) { +static Service *service_for_path(Manager *m, Path *path, const char *service_name) { _cleanup_free_ char *tmp = NULL; Unit *service_unit = NULL; - Service *service = NULL; - usec_t ts; - usec_t timeout = 2 * USEC_PER_SEC; assert_se(m); - assert_se(unit); - assert_se(test_path); + assert_se(path); if (!service_name) { - assert_se(tmp = strreplace(unit->id, ".path", ".service")); + assert_se(tmp = strreplace(UNIT(path)->id, ".path", ".service")); service_unit = manager_get_unit(m, tmp); } else service_unit = manager_get_unit(m, service_name); assert_se(service_unit); - service = SERVICE(service_unit); + + return SERVICE(service_unit); +} + +static void check_states(Manager *m, Path *path, Service *service, PathState path_state, ServiceState service_state) { + usec_t ts; + usec_t timeout = 2 * USEC_PER_SEC; + + assert_se(m); + assert_se(service); ts = now(CLOCK_MONOTONIC); - /* We process events until the service related to the path has been successfully started */ - while (service->result != SERVICE_SUCCESS || service->state != SERVICE_START) { + + while (path->result != PATH_SUCCESS || service->result != SERVICE_SUCCESS || + path->state != path_state || service->state != service_state) { usec_t n; int r; @@ -90,119 +96,216 @@ static void check_stop_unlink(Manager *m, Unit *unit, const char *test_path, con assert_se(r >= 0); printf("%s: state = %s; result = %s \n", - service_unit->id, + UNIT(path)->id, + path_state_to_string(path->state), + path_result_to_string(path->result)); + printf("%s: state = %s; result = %s \n", + UNIT(service)->id, service_state_to_string(service->state), service_result_to_string(service->result)); - /* But we timeout if the service has not been started in the allocated time */ n = now(CLOCK_MONOTONIC); if (ts + timeout < n) { - log_error("Test timeout when testing %s", unit->id); + log_error("Test timeout when testing %s", UNIT(path)->id); exit(EXIT_FAILURE); } } - - assert_se(unit_stop(unit) >= 0); - (void) rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL); } static void test_path_exists(Manager *m) { const char *test_path = "/tmp/test-path_exists"; Unit *unit = NULL; + Path *path = NULL; + Service *service = NULL; assert_se(m); assert_se(manager_load_startable_unit_or_warn(m, "path-exists.path", NULL, &unit) >= 0); + + path = PATH(unit); + service = service_for_path(m, path, NULL); + assert_se(unit_start(unit) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); assert_se(touch(test_path) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); - check_stop_unlink(m, unit, test_path, NULL); + /* Service restarts if file still exists */ + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + + assert_se(rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + + assert_se(unit_stop(unit) >= 0); } static void test_path_existsglob(Manager *m) { const char *test_path = "/tmp/test-path_existsglobFOOBAR"; Unit *unit = NULL; + Path *path = NULL; + Service *service = NULL; assert_se(m); + assert_se(manager_load_startable_unit_or_warn(m, "path-existsglob.path", NULL, &unit) >= 0); + + path = PATH(unit); + service = service_for_path(m, path, NULL); + assert_se(unit_start(unit) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); assert_se(touch(test_path) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); - check_stop_unlink(m, unit, test_path, NULL); + /* Service restarts if file still exists */ + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + + assert_se(rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + + assert_se(unit_stop(unit) >= 0); } static void test_path_changed(Manager *m) { const char *test_path = "/tmp/test-path_changed"; FILE *f; Unit *unit = NULL; + Path *path = NULL; + Service *service = NULL; assert_se(m); - assert_se(touch(test_path) >= 0); - assert_se(manager_load_startable_unit_or_warn(m, "path-changed.path", NULL, &unit) >= 0); + + path = PATH(unit); + service = service_for_path(m, path, NULL); + assert_se(unit_start(unit) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + + assert_se(touch(test_path) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + + /* Service does not restart if file still exists */ + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); f = fopen(test_path, "w"); assert_se(f); fclose(f); - check_stop_unlink(m, unit, test_path, NULL); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + + (void) rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL); + assert_se(unit_stop(unit) >= 0); } static void test_path_modified(Manager *m) { _cleanup_fclose_ FILE *f = NULL; const char *test_path = "/tmp/test-path_modified"; Unit *unit = NULL; + Path *path = NULL; + Service *service = NULL; assert_se(m); - assert_se(touch(test_path) >= 0); - assert_se(manager_load_startable_unit_or_warn(m, "path-modified.path", NULL, &unit) >= 0); + + path = PATH(unit); + service = service_for_path(m, path, NULL); + assert_se(unit_start(unit) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + + assert_se(touch(test_path) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + + /* Service does not restart if file still exists */ + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); f = fopen(test_path, "w"); assert_se(f); fputs("test", f); - check_stop_unlink(m, unit, test_path, NULL); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + + (void) rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL); + assert_se(unit_stop(unit) >= 0); } static void test_path_unit(Manager *m) { const char *test_path = "/tmp/test-path_unit"; Unit *unit = NULL; + Path *path = NULL; + Service *service = NULL; assert_se(m); assert_se(manager_load_startable_unit_or_warn(m, "path-unit.path", NULL, &unit) >= 0); + + path = PATH(unit); + service = service_for_path(m, path, "path-mycustomunit.service"); + assert_se(unit_start(unit) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); assert_se(touch(test_path) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); - check_stop_unlink(m, unit, test_path, "path-mycustomunit.service"); + assert_se(rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + + assert_se(unit_stop(unit) >= 0); } static void test_path_directorynotempty(Manager *m) { const char *test_path = "/tmp/test-path_directorynotempty/"; Unit *unit = NULL; + Path *path = NULL; + Service *service = NULL; assert_se(m); + assert_se(manager_load_startable_unit_or_warn(m, "path-directorynotempty.path", NULL, &unit) >= 0); + + path = PATH(unit); + service = service_for_path(m, path, NULL); + assert_se(access(test_path, F_OK) < 0); - assert_se(manager_load_startable_unit_or_warn(m, "path-directorynotempty.path", NULL, &unit) >= 0); assert_se(unit_start(unit) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); /* MakeDirectory default to no */ assert_se(access(test_path, F_OK) < 0); assert_se(mkdir_p(test_path, 0755) >= 0); assert_se(touch(strjoina(test_path, "test_file")) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); - check_stop_unlink(m, unit, test_path, NULL); + /* Service restarts if directory is still not empty */ + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + + assert_se(rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); + assert_se(unit_stop(UNIT(service)) >= 0); + check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + + assert_se(unit_stop(unit) >= 0); } static void test_path_makedirectory_directorymode(Manager *m) { @@ -212,9 +315,10 @@ static void test_path_makedirectory_directorymode(Manager *m) { assert_se(m); + assert_se(manager_load_startable_unit_or_warn(m, "path-makedirectory.path", NULL, &unit) >= 0); + assert_se(access(test_path, F_OK) < 0); - assert_se(manager_load_startable_unit_or_warn(m, "path-makedirectory.path", NULL, &unit) >= 0); assert_se(unit_start(unit) >= 0); /* Check if the directory has been created */ diff --git a/test/test-path/path-changed.service b/test/test-path/path-changed.service index f8499ec619..fb465d76bb 100644 --- a/test/test-path/path-changed.service +++ b/test/test-path/path-changed.service @@ -3,4 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=oneshot +Type=simple +RemainAfterExit=true diff --git a/test/test-path/path-directorynotempty.service b/test/test-path/path-directorynotempty.service index f8499ec619..fb465d76bb 100644 --- a/test/test-path/path-directorynotempty.service +++ b/test/test-path/path-directorynotempty.service @@ -3,4 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=oneshot +Type=simple +RemainAfterExit=true diff --git a/test/test-path/path-exists.service b/test/test-path/path-exists.service index f8499ec619..fb465d76bb 100644 --- a/test/test-path/path-exists.service +++ b/test/test-path/path-exists.service @@ -3,4 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=oneshot +Type=simple +RemainAfterExit=true diff --git a/test/test-path/path-existsglob.service b/test/test-path/path-existsglob.service index f8499ec619..fb465d76bb 100644 --- a/test/test-path/path-existsglob.service +++ b/test/test-path/path-existsglob.service @@ -3,4 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=oneshot +Type=simple +RemainAfterExit=true diff --git a/test/test-path/path-makedirectory.service b/test/test-path/path-makedirectory.service index f8499ec619..fb465d76bb 100644 --- a/test/test-path/path-makedirectory.service +++ b/test/test-path/path-makedirectory.service @@ -3,4 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=oneshot +Type=simple +RemainAfterExit=true diff --git a/test/test-path/path-modified.service b/test/test-path/path-modified.service index f8499ec619..fb465d76bb 100644 --- a/test/test-path/path-modified.service +++ b/test/test-path/path-modified.service @@ -3,4 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=oneshot +Type=simple +RemainAfterExit=true diff --git a/test/test-path/path-mycustomunit.service b/test/test-path/path-mycustomunit.service index 172ac0d0d5..bcdafe4f30 100644 --- a/test/test-path/path-mycustomunit.service +++ b/test/test-path/path-mycustomunit.service @@ -1,6 +1,7 @@ [Unit] -Description=Service Test Path Unit= +Description=Service Test Path Unit [Service] ExecStart=/bin/true -Type=oneshot +Type=simple +RemainAfterExit=true