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

@@ -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 = access(s->path, F_OK) >= 0;
-                good = b && !s->previous_exists;
-                s->previous_exists = b;
                 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 &&
+        if (!IN_SET(state, PATH_WAITING, PATH_RUNNING))
-            (state != PATH_RUNNING || p->inotify_triggered))
                 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 the triggered unit is already running, so are we */
-                if (path_check_good(p, initial)) {
+        trigger = UNIT_TRIGGER(UNIT(p));
-                        log_unit_debug(UNIT(p), "Got triggered.");
+        if (trigger && !UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(trigger))) {
-                        path_enter_running(p);
+                path_set_state(p, PATH_RUNNING);
-                        return;
+                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, from_trigger_notify)) {
-                if (path_check_good(p, false)) {
+                log_unit_debug(UNIT(p), "Got triggered.");
-                        log_unit_debug(UNIT(p), "Got triggered.");
+                path_enter_running(p);
-                        path_enter_running(p);
+                return;
-                        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.");
-
+                path_enter_waiting(p, false, true);
-                /* Hmm, so inotify was triggered since the
+        } else if (p->state == PATH_WAITING &&
-                 * last activation, so I guess we need to
+                   !UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) {
-                 * recheck what is going on. */
+                log_unit_debug(UNIT(p), "Got notified about unit activation.");
-                path_enter_waiting(p, false, p->inotify_triggered);
+                path_enter_waiting(p, false, true);
         }
 }
 

src/core/path.h

@@ -56,8 +56,6 @@ struct Path {
 
         PathState state, deserialized_state;
 
-        bool inotify_triggered;
-
         bool make_directory;
         mode_t directory_mode;
 

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(path);
-        assert_se(test_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 */

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

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

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

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

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

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

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