Merge pull request #4973 from poettering/run-race

run: fix race for "systemd-run --wait"

4 files changed, 88 insertions, 28 deletions

diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c
index f94959adac..4816bd1f67 100644
--- a/src/libsystemd/sd-event/sd-event.c
+++ b/src/libsystemd/sd-event/sd-event.c
@@ -2226,11 +2226,16 @@ static int process_signal(sd_event *e, struct signal_data *d, uint32_t events) {
 }
 
 static int source_dispatch(sd_event_source *s) {
+        EventSourceType saved_type;
         int r = 0;
 
         assert(s);
         assert(s->pending || s->type == SOURCE_EXIT);
 
+        /* Save the event source type, here, so that we still know it after the event callback which might invalidate
+         * the event. */
+        saved_type = s->type;
+
         if (s->type != SOURCE_DEFER && s->type != SOURCE_EXIT) {
                 r = source_set_pending(s, false);
                 if (r < 0)
@@ -2318,7 +2323,7 @@ static int source_dispatch(sd_event_source *s) {
 
         if (r < 0)
                 log_debug_errno(r, "Event source %s (type %s) returned error, disabling: %m",
-                                strna(s->description), event_source_type_to_string(s->type));
+                                strna(s->description), event_source_type_to_string(saved_type));
 
         if (s->n_ref == 0)
                 source_free(s);
diff --git a/src/run/run.c b/src/run/run.c
index 99f03465b0..08f7e12336 100644
--- a/src/run/run.c
+++ b/src/run/run.c
@@ -403,6 +403,11 @@ static int parse_argv(int argc, char *argv[]) {
                 return -EINVAL;
         }
 
+        if (arg_pty && arg_no_block) {
+                log_error("--pty is not compatible with --no-block.");
+                return -EINVAL;
+        }
+
         if (arg_scope && with_timer()) {
                 log_error("Timer options are not supported in --scope mode.");
                 return -EINVAL;
@@ -784,21 +789,23 @@ static void run_context_free(RunContext *c) {
 }
 
 static void run_context_check_done(RunContext *c) {
-        bool done = true;
+        bool done;
 
         assert(c);
 
         if (c->match)
-                done = done && (c->active_state && STR_IN_SET(c->active_state, "inactive", "failed"));
+                done = STRPTR_IN_SET(c->active_state, "inactive", "failed");
+        else
+                done = true;
 
-        if (c->forward)
-                done = done && pty_forward_is_done(c->forward);
+        if (c->forward && done) /* If the service is gone, it's time to drain the output */
+                done = pty_forward_drain(c->forward);
 
         if (done)
                 sd_event_exit(c->event, EXIT_SUCCESS);
 }
 
-static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error *error) {
+static int run_context_update(RunContext *c, const char *path) {
 
         static const struct bus_properties_map map[] = {
                 { "ActiveState",                      "s", NULL, offsetof(RunContext, active_state)        },
@@ -811,12 +818,11 @@ static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error
                 {}
         };
 
-        RunContext *c = userdata;
         int r;
 
         r = bus_map_all_properties(c->bus,
                                    "org.freedesktop.systemd1",
-                                   sd_bus_message_get_path(m),
+                                   path,
                                    map,
                                    c);
         if (r < 0) {
@@ -828,6 +834,15 @@ static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error
         return 0;
 }
 
+static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error *error) {
+        RunContext *c = userdata;
+
+        assert(m);
+        assert(c);
+
+        return run_context_update(c, sd_bus_message_get_path(m));
+}
+
 static int pty_forward_handler(PTYForward *f, int rcode, void *userdata) {
         RunContext *c = userdata;
 
@@ -985,6 +1000,8 @@ static int start_transient_service(
 
         if (arg_wait || master >= 0) {
                 _cleanup_(run_context_free) RunContext c = {};
+                _cleanup_free_ char *path = NULL;
+                const char *mt;
 
                 c.bus = sd_bus_ref(bus);
 
@@ -1007,27 +1024,27 @@ static int start_transient_service(
                         pty_forward_set_handler(c.forward, pty_forward_handler, &c);
                 }
 
-                if (arg_wait) {
-                        _cleanup_free_ char *path = NULL;
-                        const char *mt;
 
-                        path = unit_dbus_path_from_name(service);
-                        if (!path)
-                                return log_oom();
+                path = unit_dbus_path_from_name(service);
+                if (!path)
+                        return log_oom();
 
-                        mt = strjoina("type='signal',"
-                                      "sender='org.freedesktop.systemd1',"
-                                      "path='", path, "',"
-                                      "interface='org.freedesktop.DBus.Properties',"
-                                      "member='PropertiesChanged'");
-                        r = sd_bus_add_match(bus, &c.match, mt, on_properties_changed, &c);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to add properties changed signal.");
+                mt = strjoina("type='signal',"
+                              "sender='org.freedesktop.systemd1',"
+                              "path='", path, "',"
+                              "interface='org.freedesktop.DBus.Properties',"
+                              "member='PropertiesChanged'");
+                r = sd_bus_add_match(bus, &c.match, mt, on_properties_changed, &c);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to add properties changed signal.");
 
-                        r = sd_bus_attach_event(bus, c.event, 0);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to attach bus to event loop.");
-                }
+                r = sd_bus_attach_event(bus, c.event, 0);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to attach bus to event loop.");
+
+                r = run_context_update(&c, path);
+                if (r < 0)
+                        return r;
 
                 r = sd_event_loop(c.event);
                 if (r < 0)
@@ -1041,7 +1058,13 @@ static int start_transient_service(
                                 fputc('\n', stdout);
                 }
 
-                if (!arg_quiet) {
+                if (arg_wait && !arg_quiet) {
+
+                        /* Explicitly destroy the PTY forwarder, so that the PTY device is usable again, in its
+                         * original settings (i.e. proper line breaks), so that we can show the summary in a pretty
+                         * way. */
+                        c.forward = pty_forward_free(c.forward);
+
                         if (!isempty(c.result))
                                 log_info("Finished with result: %s", strna(c.result));
 
@@ -1416,7 +1439,7 @@ int main(int argc, char* argv[]) {
 
         /* If --wait is used connect via the bus, unconditionally, as ref/unref is not supported via the limited direct
          * connection */
-        if (arg_wait)
+        if (arg_wait || arg_pty)
                 r = bus_connect_transport(arg_transport, arg_host, arg_user, &bus);
         else
                 r = bus_connect_transport_systemd(arg_transport, arg_host, arg_user, &bus);
diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c
index 293c6673fc..59b541d519 100644
--- a/src/shared/ptyfwd.c
+++ b/src/shared/ptyfwd.c
@@ -69,6 +69,7 @@ struct PTYForward {
         bool read_from_master:1;
 
         bool done:1;
+        bool drain:1;
 
         bool last_char_set:1;
         char last_char;
@@ -302,6 +303,11 @@ static int shovel(PTYForward *f) {
                         return pty_forward_done(f, 0);
         }
 
+        /* If we were asked to drain, and there's nothing more to handle from the master, then call the callback
+         * too. */
+        if (f->drain && f->out_buffer_full == 0 && !f->master_readable)
+                return pty_forward_done(f, 0);
+
         return 0;
 }
 
@@ -438,6 +444,9 @@ int pty_forward_new(
                 r = sd_event_add_io(f->event, &f->stdin_event_source, STDIN_FILENO, EPOLLIN|EPOLLET, on_stdin_event, f);
                 if (r < 0 && r != -EPERM)
                         return r;
+
+                if (r >= 0)
+                        (void) sd_event_source_set_description(f->stdin_event_source, "ptyfwd-stdin");
         }
 
         r = sd_event_add_io(f->event, &f->stdout_event_source, STDOUT_FILENO, EPOLLOUT|EPOLLET, on_stdout_event, f);
@@ -446,15 +455,21 @@ int pty_forward_new(
                 f->stdout_writable = true;
         else if (r < 0)
                 return r;
+        else
+                (void) sd_event_source_set_description(f->stdout_event_source, "ptyfwd-stdout");
 
         r = sd_event_add_io(f->event, &f->master_event_source, master, EPOLLIN|EPOLLOUT|EPOLLET, on_master_event, f);
         if (r < 0)
                 return r;
 
+        (void) sd_event_source_set_description(f->master_event_source, "ptyfwd-master");
+
         r = sd_event_add_signal(f->event, &f->sigwinch_event_source, SIGWINCH, on_sigwinch_event, f);
         if (r < 0)
                 return r;
 
+        (void) sd_event_source_set_description(f->sigwinch_event_source, "ptyfwd-sigwinch");
+
         *ret = f;
         f = NULL;
 
@@ -519,3 +534,18 @@ void pty_forward_set_handler(PTYForward *f, PTYForwardHandler cb, void *userdata
         f->handler = cb;
         f->userdata = userdata;
 }
+
+bool pty_forward_drain(PTYForward *f) {
+        assert(f);
+
+        /* Starts draining the forwarder. Specifically:
+         *
+         * - Returns true if there are no unprocessed bytes from the pty, false otherwise
+         *
+         * - Makes sure the handler function is called the next time the number of unprocessed bytes hits zero
+         */
+
+        f->drain = true;
+
+        return f->out_buffer_full == 0 && !f->master_readable;
+}
diff --git a/src/shared/ptyfwd.h b/src/shared/ptyfwd.h
index bd5d5fec0d..3fad1d3b26 100644
--- a/src/shared/ptyfwd.h
+++ b/src/shared/ptyfwd.h
@@ -51,4 +51,6 @@ bool pty_forward_is_done(PTYForward *f);
 
 void pty_forward_set_handler(PTYForward *f, PTYForwardHandler handler, void *userdata);
 
+bool pty_forward_drain(PTYForward *f);
+
 DEFINE_TRIVIAL_CLEANUP_FUNC(PTYForward*, pty_forward_free);