Merge pull request #15697 from OhNoMoreGit/fix-path-units

Recheck PathExists=, PathExistsGlob=, DirectoryNotEmpty= when triggered unit terminates

diff --git a/src/core/path.c b/src/core/path.c
index 1bbf27c5c571c1fb2aba6a8cf5060c3609b67a63..1c3c28e34172c2b5484ded964e723283da82f969 100644 (file)
--- 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;
 
@@ -425,8 +423,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)
@@ -435,7 +432,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);
@@ -446,7 +443,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);
         }
@@ -486,8 +483,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);
 
@@ -498,27 +493,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)
@@ -528,12 +531,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;
@@ -579,7 +581,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;
 }
@@ -617,7 +619,7 @@ static int path_serialize(Unit *u, FILE *f, FDSet *fds) {
                 (void) serialize_item_format(f, "path-spec", "%s %i %s",
                                              type,
                                              s->previous_exists,
-                                             s->path);
+                                             escaped);
         }
 
         return 0;
@@ -728,15 +730,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;
 
@@ -760,11 +757,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 4d4b6236c27f155ce866eef120e1829fdb932ee0..9e2836535a978c2caff2350a31349e91a92f3a13 100644 (file)
--- 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 830d5f261bc32647026174f89b5e049281f7d2fa..0b2b4ab554d354bbdefc56747d17aa807cee4ead 100644 (file)
--- 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);
+
+        /* 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);
 
-        check_stop_unlink(m, unit, test_path, NULL);
+        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);
+
+        /* Service restarts if file still exists */
+        assert_se(unit_stop(UNIT(service)) >= 0);
+        check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING);
 
-        check_stop_unlink(m, unit, test_path, NULL);
+        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);
+
+        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);
 
-        check_stop_unlink(m, unit, test_path, "path-mycustomunit.service");
+        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);
+
+        /* 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);
 
-        check_stop_unlink(m, unit, test_path, NULL);
+        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 f8499ec6195d49645e9547f26ddbe11f8de7311d..fb465d76bbca5eed338988ecc969f57addbb1884 100644 (file)
--- 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 f8499ec6195d49645e9547f26ddbe11f8de7311d..fb465d76bbca5eed338988ecc969f57addbb1884 100644 (file)
--- 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 f8499ec6195d49645e9547f26ddbe11f8de7311d..fb465d76bbca5eed338988ecc969f57addbb1884 100644 (file)
--- 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 f8499ec6195d49645e9547f26ddbe11f8de7311d..fb465d76bbca5eed338988ecc969f57addbb1884 100644 (file)
--- 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 f8499ec6195d49645e9547f26ddbe11f8de7311d..fb465d76bbca5eed338988ecc969f57addbb1884 100644 (file)
--- 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 f8499ec6195d49645e9547f26ddbe11f8de7311d..fb465d76bbca5eed338988ecc969f57addbb1884 100644 (file)
--- 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 172ac0d0d5fdf12997581be564ee2c4868b58ddd..bcdafe4f3072967bb0c6d0eee853aef9e3fc3952 100644 (file)
--- 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