unit: rework a bit how we keep the service fdstore from being destroyed during servic...

When preparing for a restart we quickly go through the DEAD/INACTIVE
service state before entering AUTO_RESTART. When doing this, we need to
make sure we don't destroy the FD store. Previously this was done by
checking the failure state of the unit, and keeping the FD store around
when the unit failed, under the assumption that the restart logic will
then get into action.

This is not entirely correct howver, as there might be failure states
that will no result in restarts.

With this commit we slightly alter the logic: a ref counter for the fd
store is added, that is increased right before we handle the restart
logic, and decreased again right-after.

This should ensure that the fdstore lives exactly as long as it needs.

Follow-up for f0bfbfac43b7faa68ef1bb2ad659c191b9ec85d2.

diff --git a/src/core/service.c b/src/core/service.c
index ac3f14665d70ff5e7dcb72c24cb536f6385521de..4b912fc3b9d67822a728d0e4c85a23adf9780b34 100644 (file)
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -294,6 +294,9 @@ static void service_fd_store_unlink(ServiceFDStore *fs) {
 static void service_release_fd_store(Service *s) {
         assert(s);
 
+        if (s->n_keep_fd_store > 0)
+                return;
+
         log_unit_debug(UNIT(s), "Releasing all stored fds");
         while (s->fd_store)
                 service_fd_store_unlink(s->fd_store);
@@ -301,7 +304,7 @@ static void service_release_fd_store(Service *s) {
         assert(s->n_fd_store == 0);
 }
 
-static void service_release_resources(Unit *u, bool inactive) {
+static void service_release_resources(Unit *u) {
         Service *s = SERVICE(u);
 
         assert(s);
@@ -315,8 +318,7 @@ static void service_release_resources(Unit *u, bool inactive) {
         s->stdout_fd = safe_close(s->stdout_fd);
         s->stderr_fd = safe_close(s->stderr_fd);
 
-        if (inactive)
-                service_release_fd_store(s);
+        service_release_fd_store(s);
 }
 
 static void service_done(Unit *u) {
@@ -360,7 +362,7 @@ static void service_done(Unit *u) {
 
         s->timer_event_source = sd_event_source_unref(s->timer_event_source);
 
-        service_release_resources(u, true);
+        service_release_resources(u);
 }
 
 static int on_fd_store_io(sd_event_source *e, int fd, uint32_t revents, void *userdata) {
@@ -1524,6 +1526,10 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)
         if (s->result != SERVICE_SUCCESS)
                 log_unit_warning(UNIT(s), "Failed with result '%s'.", service_result_to_string(s->result));
 
+        /* Make sure service_release_resources() doesn't destroy our FD store, while we are changing through
+         * SERVICE_FAILED/SERVICE_DEAD before entering into SERVICE_AUTO_RESTART. */
+        s->n_keep_fd_store ++;
+
         service_set_state(s, s->result != SERVICE_SUCCESS ? SERVICE_FAILED : SERVICE_DEAD);
 
         if (s->result != SERVICE_SUCCESS)
@@ -1532,8 +1538,10 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)
         if (allow_restart && service_shall_restart(s)) {
 
                 r = service_arm_timer(s, usec_add(now(CLOCK_MONOTONIC), s->restart_usec));
-                if (r < 0)
+                if (r < 0) {
+                        s->n_keep_fd_store--;
                         goto fail;
+                }
 
                 service_set_state(s, SERVICE_AUTO_RESTART);
         } else
@@ -1541,6 +1549,11 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)
                  * user can still introspect the counter. Do so on the next start. */
                 s->flush_n_restarts = true;
 
+        /* The new state is in effect, let's decrease the fd store ref counter again. Let's also readd us to the GC
+         * queue, so that the fd store is possibly gc'ed again */
+        s->n_keep_fd_store--;
+        unit_add_to_gc_queue(UNIT(s));
+
         /* The next restart might not be a manual stop, hence reset the flag indicating manual stops */
         s->forbid_restart = false;
 

diff --git a/src/core/service.h b/src/core/service.h
index 16b700637c2f3fe33fef9ff0cb8f733fe91908a2..236d37fcbc65c8ac8d140f54ccffa13e32c130a5 100644 (file)
--- a/src/core/service.h
+++ b/src/core/service.h
@@ -186,6 +186,7 @@ struct Service {
         ServiceFDStore *fd_store;
         unsigned n_fd_store;
         unsigned n_fd_store_max;
+        unsigned n_keep_fd_store;
 
         char *usb_function_descriptors;
         char *usb_function_strings;

diff --git a/src/core/unit.c b/src/core/unit.c
index 785bd5dc441ca2b8487d15f36df89150cbe4418b..5c8dc4347ab31132b3c65919744c952595f711c7 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -331,7 +331,6 @@ int unit_set_description(Unit *u, const char *description) {
 
 bool unit_check_gc(Unit *u) {
         UnitActiveState state;
-        bool inactive;
         assert(u);
 
         if (u->job)
@@ -341,17 +340,14 @@ bool unit_check_gc(Unit *u) {
                 return true;
 
         state = unit_active_state(u);
-        inactive = state == UNIT_INACTIVE;
 
-        /* If the unit is inactive and failed and no job is queued for
-         * it, then release its runtime resources */
+        /* If the unit is inactive and failed and no job is queued for it, then release its runtime resources */
         if (UNIT_IS_INACTIVE_OR_FAILED(state) &&
             UNIT_VTABLE(u)->release_resources)
-                UNIT_VTABLE(u)->release_resources(u, inactive);
+                UNIT_VTABLE(u)->release_resources(u);
 
-        /* But we keep the unit object around for longer when it is
-         * referenced or configured to not be gc'ed */
-        if (!inactive)
+        /* But we keep the unit object around for longer when it is referenced or configured to not be gc'ed */
+        if (state != UNIT_INACTIVE)
                 return true;
 
         if (u->perpetual)

diff --git a/src/core/unit.h b/src/core/unit.h
index adab6eb80f23aec1b26ad4d2dc0b0862b852f38b..b0df341a03a7e4631d1f0b428e7fdc1bb44f283f 100644 (file)
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -453,9 +453,8 @@ struct UnitVTable {
          * way */
         bool (*check_gc)(Unit *u);
 
-        /* When the unit is not running and no job for it queued we
-         * shall release its runtime resources */
-        void (*release_resources)(Unit *u, bool inactive);
+        /* When the unit is not running and no job for it queued we shall release its runtime resources */
+        void (*release_resources)(Unit *u);
 
         /* Invoked on every child that died */
         void (*sigchld_event)(Unit *u, pid_t pid, int code, int status);