From: David Herrmann Date: Wed, 28 Nov 2012 16:47:31 +0000 (+0100) Subject: pty: fix ignoring HUP on pty-master files X-Git-Tag: kmscon-7~205 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=0b9fa6aa2914d2dd38c4773952e088a4a76163bb;p=platform%2Fupstream%2Fkmscon.git pty: fix ignoring HUP on pty-master files A HUP is reported whenever there is no client listening on the other side of the PTY. As /bin/login and friends call vhangup() during setup, there is a short time-span when we might get a HUP. Hence, we need to ignore HUPs and solely track clients by PID. To avoid hogging the CPU while the TTY is hungup, we change the event-mode to edge-triggered. This fixes a bug where we tried starting /bin/login several times because we always ran into this race-condition and directly closed the TTY again. Signed-off-by: David Herrmann --- diff --git a/src/pty.c b/src/pty.c index 74ad407..27989ff 100644 --- a/src/pty.c +++ b/src/pty.c @@ -342,7 +342,6 @@ static int pty_spawn(struct kmscon_pty *pty, int master, ws.ws_col = width; ws.ws_row = height; - log_debug("forking child"); pid = fork(); switch (pid) { case -1: @@ -353,6 +352,7 @@ static int pty_spawn(struct kmscon_pty *pty, int master, exec_child(pty->term, pty->argv, pty->seat); exit(EXIT_FAILURE); default: + log_debug("forking child %d", pid); pty->fd = master; pty->child = pid; break; @@ -375,7 +375,8 @@ static int send_buf(struct kmscon_pty *pty) } if (ret < 0 && errno != EWOULDBLOCK) { - log_warn("cannot write to child process"); + log_warn("cannot write to child process (%d): %m", + errno); return ret; } @@ -383,55 +384,77 @@ static int send_buf(struct kmscon_pty *pty) return 0; } - ev_fd_update(pty->efd, EV_READABLE); + ev_fd_update(pty->efd, EV_READABLE | EV_ET); return 0; } -static void pty_input(struct ev_fd *fd, int mask, void *data) +static int read_buf(struct kmscon_pty *pty) { - int ret; ssize_t len, num; - struct kmscon_pty *pty = data; + int mask; + + /* Use a maximum of 50 steps to avoid staying here forever. + * TODO: recheck where else a user might flush our queues and try to + * install an explicit policy. */ + num = 50; + do { + len = read(pty->fd, pty->io_buf, sizeof(pty->io_buf)); + if (len > 0) { + if (pty->input_cb) + pty->input_cb(pty, pty->io_buf, len, pty->data); + } else if (len == 0) { + log_debug("HUP during read on pty of child %d", + pty->child); + break; + } else if (errno != EWOULDBLOCK) { + log_err("cannot read from pty of child %d (%d): %m", + pty->child, errno); + break; + } + } while (len > 0 && --num); - if (mask & EV_ERR) { - log_warn("error on child pty socket"); - goto err; - } else if (mask & EV_HUP) { - log_debug("child closed remote end"); - goto err; - } + if (!num) { + log_debug("cannot read application data fast enough"); - if (mask & EV_WRITEABLE) { - ret = send_buf(pty); - if (ret) - goto err; + /* We are edge-triggered so update the mask to get the + * EV_READABLE event again next round. */ + mask = EV_READABLE | EV_ET; + if (!shl_ring_is_empty(pty->msgbuf)) + mask |= EV_WRITEABLE; + ev_fd_update(pty->efd, mask); } - if (mask & EV_READABLE) { - /* use a maximum of 50 steps to avoid staying here forever */ - num = 50; - do { - len = read(pty->fd, pty->io_buf, sizeof(pty->io_buf)); - if (len > 0) { - if (pty->input_cb) - pty->input_cb(pty, pty->io_buf, len, pty->data); - } else if (len == 0) { - log_debug("child closed remote end"); - goto err; - } else if (errno != EWOULDBLOCK) { - log_err("cannot read from pty: %m"); - goto err; - } - } while (--num && len > 0); - - if (!num) - log_debug("cannot read application data fast enough"); - } + return 0; +} - return; +static void pty_input(struct ev_fd *fd, int mask, void *data) +{ + struct kmscon_pty *pty = data; -err: - pty_close(pty, false); + /* Programs like /bin/login tend to perform a vhangup() on their TTY + * before running the login procedure. This also causes the pty master + * to get a EV_HUP event as long as no client has the TTY opened. + * This means, we cannot use the TTY connection as reliable way to track + * the client. Instead, we _must_ rely on the PID of the client to track + * them. + * However, this has the side effect that if the client forks and the + * parent exits, we loose them and restart the client. But this seems to + * be the expected behavior so we implement it here. + * Unfortunately, epoll always polls for EPOLLHUP so as long as the + * vhangup() is ongoing, we will _always_ get EPOLLHUP and cannot sleep. + * This gets worse if the client closes the TTY but doesn't exit. + * Therefore, we set the fd as edge-triggered in the epoll-set so we + * only get the events once they change. This has to be taken into + * account at all places of kmscon_pty to avoid missing events. */ + + if (mask & EV_ERR) + log_warn("error on pty socket of child %d", pty->child); + if (mask & EV_HUP) + log_debug("HUP on pty of child %d", pty->child); + if (mask & EV_WRITEABLE) + send_buf(pty); + if (mask & EV_READABLE) + read_buf(pty); } static void sig_child(struct ev_eloop *eloop, struct signalfd_siginfo *info, @@ -468,7 +491,7 @@ int kmscon_pty_open(struct kmscon_pty *pty, unsigned short width, } ret = ev_eloop_new_fd(pty->eloop, &pty->efd, master, - EV_READABLE, pty_input, pty); + EV_ET | EV_READABLE, pty_input, pty); if (ret) goto err_master; @@ -523,7 +546,7 @@ int kmscon_pty_write(struct kmscon_pty *pty, const char *u8, size_t len) u8 = &u8[ret]; } - ev_fd_update(pty->efd, EV_READABLE | EV_WRITEABLE); + ev_fd_update(pty->efd, EV_READABLE | EV_WRITEABLE | EV_ET); buf: ret = shl_ring_write(pty->msgbuf, u8, len);