From 5ce1c39f2dc990fe6ecef3e5ef52b59d49227bca Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 29 Jul 2025 04:44:41 +0900 Subject: [PATCH 1/6] ptyfwd: do not call pty_forward_done() in do_shovel() Previously, do_shovel() sometimes call pty_forward_done(), and its caller shovel() also call pty_forward_done(). Let's move all pty_forward_done() calls to shovel(), and do_shovel() not call it. No functional change, just refactoring. --- src/shared/ptyfwd.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index 255a62d4b4..eab81fca80 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -786,6 +786,18 @@ static int do_shovel(PTYForward *f) { break; } + return 0; +} + +static int shovel(PTYForward *f) { + int r; + + assert(f); + + r = do_shovel(f); + if (r < 0) + return pty_forward_done(f, r); + if (f->stdin_hangup || f->stdout_hangup || f->master_hangup) { /* Exit the loop if any side hung up and if there's * nothing more to write or nothing we could write. */ @@ -803,18 +815,6 @@ static int do_shovel(PTYForward *f) { return 0; } -static int shovel(PTYForward *f) { - int r; - - assert(f); - - r = do_shovel(f); - if (r < 0) - return pty_forward_done(f, r); - - return r; -} - static int on_master_event(sd_event_source *e, int fd, uint32_t revents, void *userdata) { PTYForward *f = ASSERT_PTR(userdata); From b823809bca6cd531a54fcf0f02427aea7cd3e651 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 29 Jul 2025 04:51:33 +0900 Subject: [PATCH 2/6] ptyfwd: split-out shovel_force() No functional change. Preparation for later change. --- src/shared/ptyfwd.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index eab81fca80..77bd624ec9 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -815,6 +815,19 @@ static int shovel(PTYForward *f) { return 0; } +static int shovel_force(PTYForward *f) { + assert(f); + + if (!f->master_hangup) + f->master_writable = f->master_readable = true; + if (!f->stdin_hangup) + f->stdin_readable = true; + if (!f->stdout_hangup) + f->stdout_writable = true; + + return shovel(f); +} + static int on_master_event(sd_event_source *e, int fd, uint32_t revents, void *userdata) { PTYForward *f = ASSERT_PTR(userdata); @@ -883,15 +896,7 @@ static int on_exit_event(sd_event_source *e, void *userdata) { if (!pty_forward_drain(f)) { /* If not drained, try to drain the buffer. */ - - if (!f->master_hangup) - f->master_writable = f->master_readable = true; - if (!f->stdin_hangup) - f->stdin_readable = true; - if (!f->stdout_hangup) - f->stdout_writable = true; - - r = shovel(f); + r = shovel_force(f); if (r < 0) return r; } From 7cd26f3560e6879b463b05babcd32c651e586f4e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 29 Jul 2025 00:59:46 +0900 Subject: [PATCH 3/6] ptyfwd: replace pty_forward_set_ignore_vhangup() with pty_forward_honor_vhangup() Currently, pty_forward_set_ignore_vhangup() is only used for disabling the flag. To make the function also disable PTY_FORWARD_IGNORE_INITIAL_VHANGUP flag, this renames it to pty_forward_honor_vhangup(). Also, for consistency, pty_forward_get_ignore_vhangup() and ignore_vhangup() are replaced with pty_forward_vhangup_honored(). --- src/machine/machinectl.c | 8 +++---- src/shared/ptyfwd.c | 50 ++++++++++++++-------------------------- src/shared/ptyfwd.h | 4 ++-- 3 files changed, 23 insertions(+), 39 deletions(-) diff --git a/src/machine/machinectl.c b/src/machine/machinectl.c index 9eddc53990..4e3138b2ff 100644 --- a/src/machine/machinectl.c +++ b/src/machine/machinectl.c @@ -1236,10 +1236,10 @@ static int on_machine_removed(sd_bus_message *m, void *userdata, sd_bus_error *r /* Tell the forwarder to exit on the next vhangup(), so that we still flush out what might be queued * and exit then. */ - r = pty_forward_set_ignore_vhangup(forward, false); + r = pty_forward_honor_vhangup(forward); if (r < 0) { /* On error, quit immediately. */ - log_error_errno(r, "Failed to set ignore_vhangup flag: %m"); + log_error_errno(r, "Failed to make PTY forwarder honor vhangup(): %m"); (void) sd_event_exit(sd_bus_get_event(sd_bus_message_get_bus(m)), EXIT_FAILURE); } @@ -1285,8 +1285,8 @@ static int process_forward(sd_event *event, sd_bus_slot *machine_removed_slot, i return log_error_errno(r, "Failed to run event loop: %m"); bool machine_died = - (flags & PTY_FORWARD_IGNORE_VHANGUP) && - pty_forward_get_ignore_vhangup(forward) == 0; + FLAGS_SET(flags, PTY_FORWARD_IGNORE_VHANGUP) && + !pty_forward_vhangup_honored(forward); if (!arg_quiet) { if (machine_died) diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index 77bd624ec9..543466ceb7 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -249,18 +249,6 @@ static RequestOperation look_for_escape(PTYForward *f, const char *buffer, size_ return REQUEST_NOP; } -static bool ignore_vhangup(PTYForward *f) { - assert(f); - - if (f->flags & PTY_FORWARD_IGNORE_VHANGUP) - return true; - - if ((f->flags & PTY_FORWARD_IGNORE_INITIAL_VHANGUP) && !f->read_from_master) - return true; - - return false; -} - static bool drained(PTYForward *f) { int q = 0; @@ -720,9 +708,9 @@ static int do_shovel(PTYForward *f) { /* Note that EIO on the master device might be caused by vhangup() or * temporary closing of everything on the other side, we treat it like EAGAIN - * here and try again, unless ignore_vhangup is off. */ + * here and try again, unless vhangup() is honored. */ - if (errno == EAGAIN || (errno == EIO && ignore_vhangup(f))) + if (errno == EAGAIN || (errno == EIO && !pty_forward_vhangup_honored(f))) f->master_readable = false; else if (IN_SET(errno, EPIPE, ECONNRESET, EIO)) { f->master_readable = f->master_writable = false; @@ -1087,33 +1075,29 @@ PTYForward* pty_forward_free(PTYForward *f) { return mfree(f); } -int pty_forward_set_ignore_vhangup(PTYForward *f, bool b) { - int r; - +int pty_forward_honor_vhangup(PTYForward *f) { assert(f); - if (FLAGS_SET(f->flags, PTY_FORWARD_IGNORE_VHANGUP) == b) - return 0; + if ((f->flags & (PTY_FORWARD_IGNORE_VHANGUP | PTY_FORWARD_IGNORE_INITIAL_VHANGUP)) == 0) + return 0; /* nothing changed. */ - SET_FLAG(f->flags, PTY_FORWARD_IGNORE_VHANGUP, b); + f->flags &= ~(PTY_FORWARD_IGNORE_VHANGUP | PTY_FORWARD_IGNORE_INITIAL_VHANGUP); - if (!ignore_vhangup(f)) { - - /* We shall now react to vhangup()s? Let's check immediately if we might be in one. */ - - f->master_readable = true; - r = shovel(f); - if (r < 0) - return r; - } - - return 0; + /* We shall now react to vhangup()s? Let's check immediately if we might be in one. */ + f->master_readable = true; + return shovel(f); } -bool pty_forward_get_ignore_vhangup(PTYForward *f) { +bool pty_forward_vhangup_honored(const PTYForward *f) { assert(f); - return FLAGS_SET(f->flags, PTY_FORWARD_IGNORE_VHANGUP); + if (FLAGS_SET(f->flags, PTY_FORWARD_IGNORE_VHANGUP)) + return false; + + if (FLAGS_SET(f->flags, PTY_FORWARD_IGNORE_INITIAL_VHANGUP) && !f->read_from_master) + return false; + + return true; } void pty_forward_set_hangup_handler(PTYForward *f, PTYForwardHangupHandler cb, void *userdata) { diff --git a/src/shared/ptyfwd.h b/src/shared/ptyfwd.h index 6f7ec0734c..eb6d2807c0 100644 --- a/src/shared/ptyfwd.h +++ b/src/shared/ptyfwd.h @@ -28,8 +28,8 @@ extern const int pty_forward_signals[N_PTY_FORWARD_SIGNALS]; int pty_forward_new(sd_event *event, int master, PTYForwardFlags flags, PTYForward **ret); PTYForward* pty_forward_free(PTYForward *f); -int pty_forward_set_ignore_vhangup(PTYForward *f, bool ignore_vhangup); -bool pty_forward_get_ignore_vhangup(PTYForward *f); +int pty_forward_honor_vhangup(PTYForward *f); +bool pty_forward_vhangup_honored(const PTYForward *f); void pty_forward_set_hangup_handler(PTYForward *f, PTYForwardHangupHandler handler, void *userdata); void pty_forward_set_hotkey_handler(PTYForward *f, PTYForwardHotkeyHandler handler, void *userdata); From 446431f5c9f098d86709abfec79a2360856510e4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 29 Jul 2025 00:47:45 +0900 Subject: [PATCH 4/6] ptyfwd: do not try to read master if already disconnected When PTYForward.done is set, the PTYForward.master is already disconnected. Let's not try to read the already closed file descriptor. Also, if we previously received vhangup, then it is not necessary to re-read the device to check vhangup, as we already know. This also make the check slightly delayed, and use a defer event source, to make the function can be called safely in another event source. --- src/shared/ptyfwd.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index 543466ceb7..bc312f1701 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -60,6 +60,7 @@ struct PTYForward { sd_event_source *master_event_source; sd_event_source *sigwinch_event_source; sd_event_source *exit_event_source; + sd_event_source *defer_event_source; struct termios saved_stdin_attr; struct termios saved_stdout_attr; @@ -122,6 +123,7 @@ static void pty_forward_disconnect(PTYForward *f) { f->master_event_source = sd_event_source_unref(f->master_event_source); f->sigwinch_event_source = sd_event_source_unref(f->sigwinch_event_source); f->exit_event_source = sd_event_source_unref(f->exit_event_source); + f->defer_event_source = sd_event_source_unref(f->defer_event_source); f->event = sd_event_unref(f->event); if (f->output_fd >= 0) { @@ -892,6 +894,23 @@ static int on_exit_event(sd_event_source *e, void *userdata) { return pty_forward_done(f, 0); } +static int on_defer_event(sd_event_source *s, void *userdata) { + PTYForward *f = ASSERT_PTR(userdata); + return shovel_force(f); +} + +static int pty_forward_add_defer(PTYForward *f) { + assert(f); + + if (f->done) + return 0; + + if (f->defer_event_source) + return sd_event_source_set_enabled(f->defer_event_source, SD_EVENT_ONESHOT); + + return sd_event_add_defer(f->event, &f->defer_event_source, on_defer_event, f); +} + int pty_forward_new( sd_event *event, int master, @@ -1083,9 +1102,11 @@ int pty_forward_honor_vhangup(PTYForward *f) { f->flags &= ~(PTY_FORWARD_IGNORE_VHANGUP | PTY_FORWARD_IGNORE_INITIAL_VHANGUP); - /* We shall now react to vhangup()s? Let's check immediately if we might be in one. */ - f->master_readable = true; - return shovel(f); + if (f->master_hangup) + return 0; + + /* We shall now react to vhangup()s? Let's check if we might be in one. */ + return pty_forward_add_defer(f); } bool pty_forward_vhangup_honored(const PTYForward *f) { From dce66b06880980c46896c7dbcde89491c704fed9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 29 Jul 2025 05:13:30 +0900 Subject: [PATCH 5/6] ptyfwd,run: make pty_forward_drain() trigger defer event to call shovel() drained() checks PTYForward.master_readable flag, but it may be tentatively unset due to a tentative error like EAGAIN in the previous IO event. Let's try to call shovel() one more time, which re-read the master and call drained() at the end. Otherwise, we may lost some data. --- src/run/run.c | 23 ++++++++++++++++------- src/shared/ptyfwd.c | 12 ++++-------- src/shared/ptyfwd.h | 2 +- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/run/run.c b/src/run/run.c index 69d0490e57..99aad5df77 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1818,17 +1818,24 @@ static int run_context_check_started(RunContext *c) { } static void run_context_check_done(RunContext *c) { + int r; + assert(c); - bool done = STRPTR_IN_SET(c->active_state, "inactive", "failed") && - !c->start_job && /* our start job */ - !c->job; /* any other job */ + if (!STRPTR_IN_SET(c->active_state, "inactive", "failed") || + c->start_job || /* our start job */ + c->job) /* any other job */ + return; - if (done && c->forward) /* If the service is gone, it's time to drain the output */ - done = pty_forward_drain(c->forward); + if (!c->forward) + return (void) sd_event_exit(c->event, EXIT_SUCCESS); - if (done) - (void) sd_event_exit(c->event, EXIT_SUCCESS); + /* If the service is gone, it's time to drain the output */ + r = pty_forward_drain(c->forward); + if (r < 0) { + log_error_errno(r, "Failed to drain PTY forwarder: %m"); + return (void) sd_event_exit(c->event, EXIT_FAILURE); + } } static int map_job(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) { @@ -1983,6 +1990,8 @@ static int pty_forward_handler(PTYForward *f, int rcode, void *userdata) { return log_error_errno(rcode, "Error on PTY forwarding logic: %m"); } + c->forward = pty_forward_free(c->forward); + run_context_check_done(c); return 0; } diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index bc312f1701..2b1154f5fa 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -1135,18 +1135,14 @@ void pty_forward_set_hotkey_handler(PTYForward *f, PTYForwardHotkeyHandler cb, v f->hotkey_userdata = userdata; } -bool pty_forward_drain(PTYForward *f) { +int 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 - */ + /* Starts draining the forwarder. This makes sure the handler function is called the next time the + * number of unprocessed bytes hits zero. */ f->drain = true; - return drained(f); + return pty_forward_add_defer(f); } int pty_forward_set_priority(PTYForward *f, int64_t priority) { diff --git a/src/shared/ptyfwd.h b/src/shared/ptyfwd.h index eb6d2807c0..1983a1cd16 100644 --- a/src/shared/ptyfwd.h +++ b/src/shared/ptyfwd.h @@ -34,7 +34,7 @@ bool pty_forward_vhangup_honored(const PTYForward *f); void pty_forward_set_hangup_handler(PTYForward *f, PTYForwardHangupHandler handler, void *userdata); void pty_forward_set_hotkey_handler(PTYForward *f, PTYForwardHotkeyHandler handler, void *userdata); -bool pty_forward_drain(PTYForward *f); +int pty_forward_drain(PTYForward *f); int pty_forward_set_priority(PTYForward *f, int64_t priority); From b749f77ad0acaed6704fbc5353d2d5352e243173 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 29 Jul 2025 04:34:01 +0900 Subject: [PATCH 6/6] run: make PTY forwarder honor vhangup() after service finished Like we already do in machinectl. --- src/run/run.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/run/run.c b/src/run/run.c index 99aad5df77..9f9e56a3c1 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1836,6 +1836,14 @@ static void run_context_check_done(RunContext *c) { log_error_errno(r, "Failed to drain PTY forwarder: %m"); return (void) sd_event_exit(c->event, EXIT_FAILURE); } + + /* Tell the forwarder to exit on the next vhangup(), so that we still flush out what might be queued + * and exit then. */ + r = pty_forward_honor_vhangup(c->forward); + if (r < 0) { + log_error_errno(r, "Failed to make PTY forwarder honor vhangup(): %m"); + return (void) sd_event_exit(c->event, EXIT_FAILURE); + } } static int map_job(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) {