run: exit early in --pty if service failed

This reworks systemd-run so that in --pty mode we watch the unit state
the way we do it in --wait mode. Whenever we notice that the service is
in failed or inactive state finish right-away, but first write all
unwritten characters we can read from the master TTY device.

This makes sure that when the TTY service fails before it opens the
slave PTY device we properly notice that and exit early, so that borked
start parameters result in immediate systemd-run failure. Previously,
we'd not notice this at all, as a PTY slave that never was opened won't
result in POLLHUP events, and we'd hence simply keep reading from it
forever.

In essence, --pty now enables the same unit watching logic that --wait
enables. However, unless --wait is specified we won#t show the final
summary, hence the effective difference should be pretty minimal.

Fixes: #3915

diff --git a/src/run/run.c b/src/run/run.c
index d3d491e..08f7e12 100644 (file)
--- a/src/run/run.c
+++ b/src/run/run.c
@@ -789,15 +789,17 @@ 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);
@@ -998,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);
 
@@ -1020,31 +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 = run_context_update(&c, path);
+                if (r < 0)
+                        return r;
 
                 r = sd_event_loop(c.event);
                 if (r < 0)
@@ -1058,7 +1058,7 @@ 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
@@ -1439,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 3a02ae9..59b541d 100644 (file)
--- 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;
 }
 
@@ -528,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 bd5d5fe..3fad1d3 100644 (file)
--- 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);