core: introduce UNIT_ATOM_PROPAGATE_STOP_GRACEFUL for PropagatesStopTo=

Follow-up for 017a7ba4f406adcf69d6b3ec15b9f2d9ed5ad853

Before this commit, when a unit that is restarting propagates stop
to other units, it can also depend on them, which results in
job type conflict and thus failure to pull in the dependencies.

So, let's introduce a new dependency atom UNIT_ATOM_PROPAGATE_STOP_GRACEFUL,
and use it for PropagatesStopTo=. It will enqueue a restart job if
there's already a start job, which meets the ultimate goal and avoids
job type conflict.

Fixes #26839

diff --git a/src/core/transaction.c b/src/core/transaction.c
index ba8f19bca17612e5c6a0c77a2c07cc31731410a8..aedd24949edfaf8ccb49650b166b1c2ec32979d6 100644 (file)
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -1055,21 +1055,15 @@ int transaction_add_job_and_dependencies(
                 }
         }
 
-        _cleanup_set_free_ Set *propagated_restart = NULL;
+        if (IN_SET(type, JOB_RESTART, JOB_STOP) || (type == JOB_START && FLAGS_SET(flags, TRANSACTION_PROPAGATE_START_AS_RESTART))) {
+                bool is_stop = type == JOB_STOP;
 
-        if (type == JOB_RESTART || (type == JOB_START && FLAGS_SET(flags, TRANSACTION_PROPAGATE_START_AS_RESTART))) {
-
-                /* We propagate RESTART only as TRY_RESTART, in order not to start dependencies that
-                 * are not around. */
-
-                UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PROPAGATE_RESTART) {
+                UNIT_FOREACH_DEPENDENCY(dep, ret->unit, is_stop ? UNIT_ATOM_PROPAGATE_STOP : UNIT_ATOM_PROPAGATE_RESTART) {
+                        /* We propagate RESTART only as TRY_RESTART, in order not to start dependencies that
+                         * are not around. */
                         JobType nt;
 
-                        r = set_ensure_put(&propagated_restart, NULL, dep);
-                        if (r < 0)
-                                return r;
-
-                        nt = job_type_collapse(JOB_TRY_RESTART, dep);
+                        nt = job_type_collapse(is_stop ? JOB_STOP : JOB_TRY_RESTART, dep);
                         if (nt == JOB_NOP)
                                 continue;
 
@@ -1081,24 +1075,49 @@ int transaction_add_job_and_dependencies(
                                 sd_bus_error_free(e);
                         }
                 }
-        }
 
-        if (type == JOB_STOP) {
-                /* The 'stop' part of a restart job is also propagated to units with UNIT_ATOM_PROPAGATE_STOP. */
-
-                UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PROPAGATE_STOP) {
-                        /* Units experienced restart propagation are skipped */
-                        if (set_contains(propagated_restart, dep))
-                                continue;
+                /* Process UNIT_ATOM_PROPAGATE_STOP_GRACEFUL (PropagatesStopTo=) units. We need to wait until
+                 * all other dependencies are processed, i.e. we're the anchor job or already in the recursion
+                 * that handles it. */
+                if (!by || FLAGS_SET(flags, TRANSACTION_PROCESS_PROPAGATE_STOP_GRACEFUL))
+                        UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PROPAGATE_STOP_GRACEFUL) {
+                                JobType nt = JOB_STOP;
+                                Job *j;
+
+                                j = hashmap_get(tr->jobs, dep);
+                                if (j)
+                                        LIST_FOREACH(transaction, i, j)
+                                                switch (i->type) {
+
+                                                case JOB_STOP:
+                                                case JOB_RESTART:
+                                                        /* Nothing to worry about, an appropriate job is in-place */
+                                                        nt = JOB_NOP;
+                                                        break;
+
+                                                case JOB_START:
+                                                        /* This unit is pulled in by other dependency types in
+                                                         * this transaction. We will run into job type conflict
+                                                         * if we enqueue a stop job, so let's enqueue a restart
+                                                         * job instead. */
+                                                        nt = JOB_RESTART;
+
+                                                default: /* We don't care about others */
+                                                        ;
+
+                                                }
+
+                                if (nt == JOB_NOP)
+                                        continue;
 
-                        r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e);
-                        if (r < 0) {
-                                if (r != -EBADR) /* job type not applicable */
-                                        return r;
+                                r = transaction_add_job_and_dependencies(tr, nt, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER) | TRANSACTION_PROCESS_PROPAGATE_STOP_GRACEFUL, e);
+                                if (r < 0) {
+                                        if (r != -EBADR) /* job type not applicable */
+                                                return r;
 
-                                sd_bus_error_free(e);
+                                        sd_bus_error_free(e);
+                                }
                         }
-                }
         }
 
         if (type == JOB_RELOAD)

diff --git a/src/core/transaction.h b/src/core/transaction.h
index db2d0566499a2f18cb635802286454c57a65e369..151e02dd605e07d6bbcee31b3f3583f5302f4f6d 100644 (file)
--- a/src/core/transaction.h
+++ b/src/core/transaction.h
@@ -21,13 +21,16 @@ Transaction *transaction_abort_and_free(Transaction *tr);
 DEFINE_TRIVIAL_CLEANUP_FUNC(Transaction*, transaction_abort_and_free);
 
 typedef enum TransactionAddFlags {
-        TRANSACTION_MATTERS                    = 1 << 0,
-        TRANSACTION_CONFLICTS                  = 1 << 1,
-        TRANSACTION_IGNORE_REQUIREMENTS        = 1 << 2,
-        TRANSACTION_IGNORE_ORDER               = 1 << 3,
+        TRANSACTION_MATTERS                         = 1 << 0,
+        TRANSACTION_CONFLICTS                       = 1 << 1,
+        TRANSACTION_IGNORE_REQUIREMENTS             = 1 << 2,
+        TRANSACTION_IGNORE_ORDER                    = 1 << 3,
 
         /* Propagate a START job to other units like a RESTART */
-        TRANSACTION_PROPAGATE_START_AS_RESTART = 1 << 4,
+        TRANSACTION_PROPAGATE_START_AS_RESTART      = 1 << 4,
+
+        /* Indicate that we're in the recursion for processing UNIT_ATOM_PROPAGATE_STOP_GRACEFUL units */
+        TRANSACTION_PROCESS_PROPAGATE_STOP_GRACEFUL = 1 << 5,
 } TransactionAddFlags;
 
 void transaction_add_propagate_reload_jobs(

diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c
index 81333523e73865ee8166ae3fe5fc2a33a3040fbc..35b279b5c743967cb291e051a3daf16125a0c2b7 100644 (file)
--- a/src/core/unit-dependency-atom.c
+++ b/src/core/unit-dependency-atom.c
@@ -80,7 +80,7 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = {
                                         UNIT_ATOM_PROPAGATE_STOP_FAILURE,
 
         [UNIT_PROPAGATES_STOP_TO]     = UNIT_ATOM_RETROACTIVE_STOP_ON_STOP |
-                                        UNIT_ATOM_PROPAGATE_STOP,
+                                        UNIT_ATOM_PROPAGATE_STOP_GRACEFUL,
 
         /* These are simple dependency types: they consist of a single atom only */
         [UNIT_ON_FAILURE]             = UNIT_ATOM_ON_FAILURE,
@@ -196,6 +196,11 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) {
         case UNIT_ATOM_PROPAGATE_STOP_FAILURE:
                 return UNIT_CONFLICTED_BY;
 
+        case UNIT_ATOM_RETROACTIVE_STOP_ON_STOP |
+                UNIT_ATOM_PROPAGATE_STOP_GRACEFUL:
+        case UNIT_ATOM_PROPAGATE_STOP_GRACEFUL:
+                return UNIT_PROPAGATES_STOP_TO;
+
         /* And now, the simple ones */
 
         case UNIT_ATOM_ON_FAILURE:

diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h
index 02532e57d6a17eb2fef0954a39183f61303667f3..96f00ca39a89d802227f513f5d39d196a5b92fc6 100644 (file)
--- a/src/core/unit-dependency-atom.h
+++ b/src/core/unit-dependency-atom.h
@@ -58,30 +58,33 @@ typedef enum UnitDependencyAtom {
         UNIT_ATOM_PROPAGATE_INACTIVE_START_AS_FAILURE = UINT64_C(1) << 17,
         /* When putting together a transaction, propagate JOB_STOP from our unit to the other. */
         UNIT_ATOM_PROPAGATE_STOP                      = UINT64_C(1) << 18,
+        /* Like UNIT_ATOM_PROPAGATE_STOP, but enqueues a restart job if there's already a start job (avoids
+         * job type conflict). */
+        UNIT_ATOM_PROPAGATE_STOP_GRACEFUL             = UINT64_C(1) << 19,
         /* When putting together a transaction, propagate JOB_RESTART from our unit to the other. */
-        UNIT_ATOM_PROPAGATE_RESTART                   = UINT64_C(1) << 19,
+        UNIT_ATOM_PROPAGATE_RESTART                   = UINT64_C(1) << 20,
 
         /* Add the other unit to the default target dependency queue */
-        UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE = UINT64_C(1) << 20,
+        UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE = UINT64_C(1) << 21,
         /* Recheck default target deps on other units (which are target units) */
-        UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES         = UINT64_C(1) << 21,
+        UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES         = UINT64_C(1) << 22,
 
         /* The remaining atoms map 1:1 to the equally named high-level deps */
-        UNIT_ATOM_ON_FAILURE                          = UINT64_C(1) << 22,
-        UNIT_ATOM_ON_SUCCESS                          = UINT64_C(1) << 23,
-        UNIT_ATOM_ON_FAILURE_OF                       = UINT64_C(1) << 24,
-        UNIT_ATOM_ON_SUCCESS_OF                       = UINT64_C(1) << 25,
-        UNIT_ATOM_BEFORE                              = UINT64_C(1) << 26,
-        UNIT_ATOM_AFTER                               = UINT64_C(1) << 27,
-        UNIT_ATOM_TRIGGERS                            = UINT64_C(1) << 28,
-        UNIT_ATOM_TRIGGERED_BY                        = UINT64_C(1) << 29,
-        UNIT_ATOM_PROPAGATES_RELOAD_TO                = UINT64_C(1) << 30,
-        UNIT_ATOM_JOINS_NAMESPACE_OF                  = UINT64_C(1) << 31,
-        UNIT_ATOM_REFERENCES                          = UINT64_C(1) << 32,
-        UNIT_ATOM_REFERENCED_BY                       = UINT64_C(1) << 33,
-        UNIT_ATOM_IN_SLICE                            = UINT64_C(1) << 34,
-        UNIT_ATOM_SLICE_OF                            = UINT64_C(1) << 35,
-        _UNIT_DEPENDENCY_ATOM_MAX                     = (UINT64_C(1) << 36) - 1,
+        UNIT_ATOM_ON_FAILURE                          = UINT64_C(1) << 23,
+        UNIT_ATOM_ON_SUCCESS                          = UINT64_C(1) << 24,
+        UNIT_ATOM_ON_FAILURE_OF                       = UINT64_C(1) << 25,
+        UNIT_ATOM_ON_SUCCESS_OF                       = UINT64_C(1) << 26,
+        UNIT_ATOM_BEFORE                              = UINT64_C(1) << 27,
+        UNIT_ATOM_AFTER                               = UINT64_C(1) << 28,
+        UNIT_ATOM_TRIGGERS                            = UINT64_C(1) << 29,
+        UNIT_ATOM_TRIGGERED_BY                        = UINT64_C(1) << 30,
+        UNIT_ATOM_PROPAGATES_RELOAD_TO                = UINT64_C(1) << 31,
+        UNIT_ATOM_JOINS_NAMESPACE_OF                  = UINT64_C(1) << 32,
+        UNIT_ATOM_REFERENCES                          = UINT64_C(1) << 33,
+        UNIT_ATOM_REFERENCED_BY                       = UINT64_C(1) << 34,
+        UNIT_ATOM_IN_SLICE                            = UINT64_C(1) << 35,
+        UNIT_ATOM_SLICE_OF                            = UINT64_C(1) << 36,
+        _UNIT_DEPENDENCY_ATOM_MAX                     = (UINT64_C(1) << 37) - 1,
         _UNIT_DEPENDENCY_ATOM_INVALID                 = -EINVAL,
 } UnitDependencyAtom;