From 7c39b498d6ea21b1c6f4d26b74c064ef2b507706 Mon Sep 17 00:00:00 2001

From: Lennart Poettering <lennart@poettering.net>

Date: Mon, 13 Nov 2017 15:08:49 +0100

Subject: [PATCH] unit: rework a bit how we keep the service fdstore from being

 destroyed during service restart

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.

(cherry picked from commit 7eb2a8a1259043e107ebec94e30ed160a93f40a7)

(cherry picked from commit e2636bde0f07319d0d35262dac6ff2638ba4e598)

(cherry picked from commit 20b18b7b9aac2532215dba4d56f19989a5fca9f0)

Related: #1798162

---

 src/core/service.c | 23 ++++++++++++++++++-----

 src/core/service.h |  1 +

 src/core/unit.c    | 12 ++++--------

 src/core/unit.h    |  5 ++---

 4 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/src/core/service.c b/src/core/service.c

index ad45a38270..283c2968ea 100644

--- a/src/core/service.c

+++ b/src/core/service.c

@@ -265,6 +265,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)->id, "Releasing all stored fds");

         while (s->fd_store)

                 service_fd_store_unlink(s->fd_store);

@@ -272,7 +275,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);

@@ -282,8 +285,7 @@ static void service_release_resources(Unit *u, bool inactive) {

         log_unit_debug(u->id, "Releasing resources.");

-        if (inactive)

-                service_release_fd_store(s);

+        service_release_fd_store(s);

 }

 static void service_done(Unit *u) {

@@ -331,7 +333,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) {

@@ -1354,6 +1356,10 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)

         if (f != SERVICE_SUCCESS)

                 s->result = f;

+        /* 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) {

@@ -1375,12 +1381,19 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)

             (!IN_SET(s->main_exec_status.code, CLD_KILLED, CLD_DUMPED) || !set_contains(s->restart_prevent_status.signal, INT_TO_PTR(s->main_exec_status.status)))) {

                 r = service_arm_timer(s, s->restart_usec);

-                if (r < 0)

+                if (r < 0) {

+                        s->n_keep_fd_store--;

                         goto fail;

+                }

                 service_set_state(s, SERVICE_AUTO_RESTART);

         }

+        /* 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));

+

         s->forbid_restart = false;

         /* We want fresh tmpdirs in case service is started again immediately */

diff --git a/src/core/service.h b/src/core/service.h

index e0547a464e..81b17db652 100644

--- a/src/core/service.h

+++ b/src/core/service.h

@@ -214,6 +214,7 @@ struct Service {

         ServiceFDStore *fd_store;

         unsigned n_fd_store;

         unsigned n_fd_store_max;

+        unsigned n_keep_fd_store;

 };

 extern const UnitVTable service_vtable;

diff --git a/src/core/unit.c b/src/core/unit.c

index 33e0b7126b..e74c6bc2cc 100644

--- a/src/core/unit.c

+++ b/src/core/unit.c

@@ -284,7 +284,6 @@ int unit_set_description(Unit *u, const char *description) {

 bool unit_may_gc(Unit *u) {

         UnitActiveState state;

-        bool inactive;

         assert(u);

         /* Checks whether the unit is ready to be unloaded for garbage collection.

@@ -302,17 +301,14 @@ bool unit_may_gc(Unit *u) {

                 return false;

         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 false;

         if (UNIT_VTABLE(u)->no_gc)

diff --git a/src/core/unit.h b/src/core/unit.h

index 97a63f0f8b..61ff59e8c7 100644

--- a/src/core/unit.h

+++ b/src/core/unit.h

@@ -357,9 +357,8 @@ struct UnitVTable {

          * even though nothing references it and it isn't active in any way. */

         bool (*may_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);

         /* Return true when this unit is suitable for snapshotting */

         bool (*check_snapshot)(Unit *u);