From 2153899d96486966867b5dac6343bd579393ff7a Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 19 May 2024 09:16:56 +0800 Subject: [PATCH 1/4] ptyfwd: trivial coding style and log cleanups --- src/shared/ptyfwd.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index 897e39c533..d9ed74b950 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -570,7 +570,7 @@ static int do_shovel(PTYForward *f) { f->stdin_event_source = sd_event_source_unref(f->stdin_event_source); } else - return log_error_errno(errno, "read(): %m"); + return log_error_errno(errno, "Failed to read from pty input fd: %m"); } else if (k == 0) { /* EOF on stdin */ f->stdin_readable = false; @@ -625,7 +625,7 @@ static int do_shovel(PTYForward *f) { f->master_event_source = sd_event_source_unref(f->master_event_source); } else - return log_error_errno(errno, "read(): %m"); + return log_error_errno(errno, "Failed to read from pty master fd: %m"); } else { f->read_from_master = true; size_t scan_index = f->out_buffer_full; @@ -649,7 +649,7 @@ static int do_shovel(PTYForward *f) { f->stdout_hangup = true; f->stdout_event_source = sd_event_source_unref(f->stdout_event_source); } else - return log_error_errno(errno, "write(): %m"); + return log_error_errno(errno, "Failed to write to pty output fd: %m"); } else { @@ -767,7 +767,7 @@ int pty_forward_new( if (!f) return -ENOMEM; - *f = (struct PTYForward) { + *f = (PTYForward) { .flags = flags, .master = -EBADF, .input_fd = -EBADF, @@ -845,14 +845,15 @@ int pty_forward_new( (void) ioctl(master, TIOCSWINSZ, &ws); - if (!(flags & PTY_FORWARD_READ_ONLY)) { - int same; + if (!FLAGS_SET(flags, PTY_FORWARD_READ_ONLY)) { + bool same; assert(f->input_fd >= 0); - same = fd_inode_same(f->input_fd, f->output_fd); - if (same < 0) - return same; + r = fd_inode_same(f->input_fd, f->output_fd); + if (r < 0) + return r; + same = r > 0; if (tcgetattr(f->input_fd, &f->saved_stdin_attr) >= 0) { struct termios raw_stdin_attr; @@ -917,10 +918,12 @@ int pty_forward_new( PTYForward *pty_forward_free(PTYForward *f) { if (!f) return NULL; + pty_forward_disconnect(f); free(f->background_color); free(f->title); free(f->title_prefix); + return mfree(f); } @@ -940,15 +943,14 @@ int pty_forward_set_ignore_vhangup(PTYForward *f, bool b) { assert(f); - if (!!(f->flags & PTY_FORWARD_IGNORE_VHANGUP) == b) + if (FLAGS_SET(f->flags, PTY_FORWARD_IGNORE_VHANGUP) == b) return 0; SET_FLAG(f->flags, PTY_FORWARD_IGNORE_VHANGUP, b); if (!ignore_vhangup(f)) { - /* We shall now react to vhangup()s? Let's check - * immediately if we might be in one */ + /* We shall now react to vhangup()s? Let's check immediately if we might be in one. */ f->master_readable = true; r = shovel(f); @@ -962,7 +964,7 @@ int pty_forward_set_ignore_vhangup(PTYForward *f, bool b) { bool pty_forward_get_ignore_vhangup(PTYForward *f) { assert(f); - return !!(f->flags & PTY_FORWARD_IGNORE_VHANGUP); + return FLAGS_SET(f->flags, PTY_FORWARD_IGNORE_VHANGUP); } bool pty_forward_is_done(PTYForward *f) { @@ -994,6 +996,7 @@ bool pty_forward_drain(PTYForward *f) { int pty_forward_set_priority(PTYForward *f, int64_t priority) { int r; + assert(f); if (f->stdin_event_source) { From 6d2a26a1367b0203d38e22132eaa96a05ed1438a Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 19 May 2024 07:59:24 +0800 Subject: [PATCH 2/4] vmspawn: use openpt_allocate where appropriate --- src/vmspawn/vmspawn.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index 4a6ef759a5..326722db05 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -1572,16 +1572,9 @@ static int run_virtual_machine(int kvm_device_fd, int vhost_device_fd) { case CONSOLE_INTERACTIVE: { _cleanup_free_ char *pty_path = NULL; - master = posix_openpt(O_RDWR|O_NOCTTY|O_CLOEXEC|O_NONBLOCK); + master = openpt_allocate(O_RDWR|O_NONBLOCK, &pty_path); if (master < 0) - return log_error_errno(errno, "Failed to acquire pseudo tty: %m"); - - r = ptsname_malloc(master, &pty_path); - if (r < 0) - return log_error_errno(r, "Failed to determine tty name: %m"); - - if (unlockpt(master) < 0) - return log_error_errno(errno, "Failed to unlock tty: %m"); + return log_error_errno(master, "Failed to setup pty: %m"); if (strv_extend_many( &cmdline, From ade0789fabbf01b95bf54d32f8cab1217a753f03 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 19 May 2024 08:53:07 +0800 Subject: [PATCH 3/4] run: do not log "Error on PTY forwarding logic" when disconnected due to user operation --- src/run/run.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/run/run.c b/src/run/run.c index dc4486490b..dfb0c84144 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1496,11 +1496,13 @@ static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error } static int pty_forward_handler(PTYForward *f, int rcode, void *userdata) { - RunContext *c = userdata; + RunContext *c = ASSERT_PTR(userdata); assert(f); - if (rcode < 0) { + if (rcode == -ECANCELED) + log_debug_errno(rcode, "PTY forwarder disconnected."); + else if (rcode < 0) { sd_event_exit(c->event, EXIT_FAILURE); return log_error_errno(rcode, "Error on PTY forwarding logic: %m"); } From 28459ba1f4df824d5ef7f7d1a9acb6953ea24045 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 19 May 2024 09:07:21 +0800 Subject: [PATCH 4/4] run: pass the pty slave fd to transient service The rationale is similar to 40e1f4ea7458a0a80eaf1ef356e52bfe0835412e. Currently, we only pass TTYPath=/dev/pts/... to the transient service spawned by systemd-run. This is a bit problematic though, when ExecStartPre= or ExecStopPost= is used. Since when these control processes get to run, the main process is not yet started/has already exited, hence the slave suffers from the same vhangup problem as the mentioned commit. By passing the slave fd in, the service manager will hold the fd open as long as the service is alive. Fixes #32916 --- src/run/run.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/run/run.c b/src/run/run.c index dfb0c84144..fa77c5ea78 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1133,11 +1133,17 @@ static int transient_service_set_properties(sd_bus_message *m, const char *pty_p } if (pty_path) { + _cleanup_close_ int pty_slave = -EBADF; + + pty_slave = open_terminal(pty_path, O_RDWR|O_NOCTTY|O_CLOEXEC); + if (pty_slave < 0) + return pty_slave; + r = sd_bus_message_append(m, "(sv)(sv)(sv)(sv)", - "StandardInput", "s", "tty", - "StandardOutput", "s", "tty", - "StandardError", "s", "tty", + "StandardInputFileDescriptor", "h", pty_slave, + "StandardOutputFileDescriptor", "h", pty_slave, + "StandardErrorFileDescriptor", "h", pty_slave, "TTYPath", "s", pty_path); if (r < 0) return bus_log_create_error(r);