From dfd79eca55e3d4bd61813ca4cf8887544d952c28 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Koutn=C3=BD?= <mkoutny@suse.com>
Date: Thu, 6 Jun 2019 23:27:20 +0200
Subject: [PATCH] core: Check transaction against execution cycles

When we are validating a transaction, we take into account declared
ordering between job units. However, since JOB_STOP goes always first
regardless of the ordering constraint between respective units, we may
detect some false cycles in the transaction which would not prevent the
execution though.

Use the same logic in transaction checking as we use for job execution.
---
 src/core/transaction.c | 45 +++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/src/core/transaction.c b/src/core/transaction.c
index 3b6b240d36..d1bf2e64c9 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -353,6 +353,11 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi
         Unit *u;
         void *v;
         int r;
+        static const UnitDependency directions[] = {
+                UNIT_BEFORE,
+                UNIT_AFTER,
+        };
+        size_t d;
 
         assert(tr);
         assert(j);
@@ -441,25 +446,33 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi
         j->marker = from ? from : j;
         j->generation = generation;
 
-        /* We assume that the dependencies are bidirectional, and
-         * hence can ignore UNIT_AFTER */
-        HASHMAP_FOREACH_KEY(v, u, j->unit->dependencies[UNIT_BEFORE], i) {
-                Job *o;
+        /* Actual ordering of jobs depends on the unit ordering dependency and job types. We need to traverse
+         * the graph over 'before' edges in the actual job execution order. We traverse over both unit
+         * ordering dependencies and we test with job_compare() whether it is the 'before' edge in the job
+         * execution ordering. */
+        for (d = 0; d < ELEMENTSOF(directions); d++) {
+                HASHMAP_FOREACH_KEY(v, u, j->unit->dependencies[directions[d]], i) {
+                        Job *o;
 
-                /* Is there a job for this unit? */
-                o = hashmap_get(tr->jobs, u);
-                if (!o) {
-                        /* Ok, there is no job for this in the
-                         * transaction, but maybe there is already one
-                         * running? */
-                        o = u->job;
-                        if (!o)
+                        /* Is there a job for this unit? */
+                        o = hashmap_get(tr->jobs, u);
+                        if (!o) {
+                                /* Ok, there is no job for this in the
+                                 * transaction, but maybe there is already one
+                                 * running? */
+                                o = u->job;
+                                if (!o)
+                                        continue;
+                        }
+
+                        /* Cut traversing if the job j is not really *before* o. */
+                        if (job_compare(j, o, directions[d]) >= 0)
                                 continue;
-                }
 
-                r = transaction_verify_order_one(tr, o, j, generation, e);
-                if (r < 0)
-                        return r;
+                        r = transaction_verify_order_one(tr, o, j, generation, e);
+                        if (r < 0)
+                                return r;
+                }
         }
 
         /* Ok, let's backtrack, and remember that this entry is not on