portablectl: block when stopping a unit on detach (--now)

If portablectl detach --now is used, there's a possible race condition
where the unit is not stopped in time before the detach is attempted,
which causes it to fail.
Add a DBUS call to block after starting/stopping if --now is passed,
and add a --no-block parameter to skip it optionally when starting,
since it is not necessary in that case for correct functioning.

2 files changed, 52 insertions, 6 deletions

diff --git a/man/portablectl.xml b/man/portablectl.xml
index d1790db2fa..f2d8da40c4 100644
--- a/man/portablectl.xml
+++ b/man/portablectl.xml
@@ -135,7 +135,8 @@
         the service manager are seen by it.</para>
 
         <para>If <option>--now</option> and/or <option>--enable</option> are passed, the portable service(s) are
-        immediately started and/or enabled after attaching the image.</para>
+        immediately started (blocking operation unless <option>--no-block</option> is passed) and/or enabled after
+        attaching the image.</para>
         </listitem>
       </varlistentry>
 
@@ -150,8 +151,8 @@
         <command>detach</command>.</para></listitem>
 
         <para>If <option>--now</option> and/or <option>--enable</option> are passed, the portable service(s) are
-        immediately started and/or enabled before detaching the image. Prefix(es) are also accepted, to be used in
-        case the unit names do not match the image name as described in the <command>attach</command>.</para>
+        immediately stopped (blocking operation) and/or disabled before detaching the image. Prefix(es) are also accepted,
+        to be used in case the unit names do not match the image name as described in the <command>attach</command>.</para>
       </varlistentry>
 
       <varlistentry>
@@ -330,6 +331,12 @@
         <listitem><para>Immediately start/stop the portable service after attaching/before detaching.</para></listitem>
       </varlistentry>
 
+      <varlistentry>
+        <term><option>--no-block</option></term>
+
+        <listitem><para>Don't block waiting for attach --now to complete.</para></listitem>
+      </varlistentry>
+
       <xi:include href="user-system-options.xml" xpointer="host" />
       <xi:include href="user-system-options.xml" xpointer="machine" />
 
diff --git a/src/portable/portablectl.c b/src/portable/portablectl.c
index deaad0a0b0..bf5badd699 100644
--- a/src/portable/portablectl.c
+++ b/src/portable/portablectl.c
@@ -9,6 +9,7 @@
 #include "bus-error.h"
 #include "bus-unit-util.h"
 #include "bus-util.h"
+#include "bus-wait-for-jobs.h"
 #include "def.h"
 #include "dirent-util.h"
 #include "env-file.h"
@@ -42,6 +43,7 @@ static BusTransport arg_transport = BUS_TRANSPORT_LOCAL;
 static const char *arg_host = NULL;
 static bool arg_enable = false;
 static bool arg_now = false;
+static bool arg_no_block = false;
 
 static int determine_image(const char *image, bool permit_non_existing, char **ret) {
         int r;
@@ -445,7 +447,7 @@ static int maybe_enable_disable(sd_bus *bus, const char *path, bool enable) {
         return 0;
 }
 
-static int maybe_start_stop(sd_bus *bus, const char *path, bool start) {
+static int maybe_start_stop(sd_bus *bus, const char *path, bool start, BusWaitForJobs *wait) {
         _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         char *name = (char *)basename(path), *job = NULL;
@@ -476,15 +478,29 @@ static int maybe_start_stop(sd_bus *bus, const char *path, bool start) {
         if (!arg_quiet)
                 log_info("Queued %s to %s portable service %s.", job, start ? "start" : "stop", name);
 
+        if (wait) {
+                r = bus_wait_for_jobs_add(wait, job);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to watch %s job for %s %s: %m",
+                                               job, start ? "starting" : "stopping", name);
+        }
+
         return 0;
 }
 
 static int maybe_enable_start(sd_bus *bus, sd_bus_message *reply) {
+        _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *wait = NULL;
         int r;
 
         if (!arg_enable && !arg_now)
                 return 0;
 
+        if (!arg_no_block) {
+                r = bus_wait_for_jobs_new(bus, &wait);
+                if (r < 0)
+                        return log_error_errno(r, "Could not watch jobs: %m");
+        }
+
         r = sd_bus_message_rewind(reply, true);
         if (r < 0)
                 return r;
@@ -503,7 +519,7 @@ static int maybe_enable_start(sd_bus *bus, sd_bus_message *reply) {
 
                 if (STR_IN_SET(type, "symlink", "copy") && ENDSWITH_SET(path, ".service", ".target", ".socket")) {
                         (void) maybe_enable_disable(bus, path, true);
-                        (void) maybe_start_stop(bus, path, true);
+                        (void) maybe_start_stop(bus, path, true, wait);
                 }
         }
 
@@ -511,10 +527,17 @@ static int maybe_enable_start(sd_bus *bus, sd_bus_message *reply) {
         if (r < 0)
                 return r;
 
+        if (!arg_no_block) {
+                r = bus_wait_for_jobs(wait, arg_quiet, NULL);
+                if (r < 0)
+                        return r;
+        }
+
         return 0;
 }
 
 static int maybe_stop_disable(sd_bus *bus, char *image, char *argv[]) {
+        _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *wait = NULL;
         _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL;
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         _cleanup_strv_free_ char **matches = NULL;
@@ -527,6 +550,10 @@ static int maybe_stop_disable(sd_bus *bus, char *image, char *argv[]) {
         if (r < 0)
                 return r;
 
+        r = bus_wait_for_jobs_new(bus, &wait);
+        if (r < 0)
+                return log_error_errno(r, "Could not watch jobs: %m");
+
         r = sd_bus_message_new_method_call(
                                 bus,
                                 &m,
@@ -578,7 +605,7 @@ static int maybe_stop_disable(sd_bus *bus, char *image, char *argv[]) {
                 if (r < 0)
                         return bus_log_parse_error(r);
 
-                (void) maybe_start_stop(bus, name, false);
+                (void) maybe_start_stop(bus, name, false, wait);
                 (void) maybe_enable_disable(bus, name, false);
         }
 
@@ -586,6 +613,11 @@ static int maybe_stop_disable(sd_bus *bus, char *image, char *argv[]) {
         if (r < 0)
                 return bus_log_parse_error(r);
 
+        /* Stopping must always block or the detach will fail if the unit is still running */
+        r = bus_wait_for_jobs(wait, arg_quiet, NULL);
+        if (r < 0)
+                return r;
+
         return 0;
 }
 
@@ -997,6 +1029,7 @@ static int help(int argc, char *argv[], void *userdata) {
                "                              after attach/detach\n"
                "     --now                    Immediately start/stop the portable service after\n"
                "                              attach/before detach\n"
+               "     --no-block               Don't block waiting for attach --now to complete\n"
                "\nSee the %s for details.\n"
                , program_invocation_short_name
                , ansi_highlight()
@@ -1020,6 +1053,7 @@ static int parse_argv(int argc, char *argv[]) {
                 ARG_CAT,
                 ARG_ENABLE,
                 ARG_NOW,
+                ARG_NO_BLOCK,
         };
 
         static const struct option options[] = {
@@ -1038,6 +1072,7 @@ static int parse_argv(int argc, char *argv[]) {
                 { "cat",             no_argument,       NULL, ARG_CAT             },
                 { "enable",          no_argument,       NULL, ARG_ENABLE          },
                 { "now",             no_argument,       NULL, ARG_NOW             },
+                { "no-block",        no_argument,       NULL, ARG_NO_BLOCK        },
                 {}
         };
 
@@ -1132,6 +1167,10 @@ static int parse_argv(int argc, char *argv[]) {
                         arg_now = true;
                         break;
 
+                case ARG_NO_BLOCK:
+                        arg_no_block = true;
+                        break;
+
                 case '?':
                         return -EINVAL;