From c596f5b73c09a914fdaa12c9098d75d7a49e7fde Mon Sep 17 00:00:00 2001 From: Takashi Yano Date: Wed, 25 Mar 2015 20:42:38 +0900 Subject: [PATCH] TIOCPKT mode of PTY is broken if ONLCR bit is cleared. * tty.h (class tty_min): Remove variable "write_error" to which any errors are not currently set at anywhere. (class tty): Add variable "column" for handling ONOCR. * tty.cc (tty::init): Add initialization code for variable "column". * fhandler.h (class fhandler_pty_master): Remove variable "need_nl" which is not necessary any more. "need_nl" was needed by OPOST process in fhandler_pty_master::process_slave_output(). (class fhandler_pty_common): Add function process_opost_output() for handling post processing for OPOST in write process. * fhandler_tty.cc (fhandler_pty_master::process_slave_output): Count TIOCPKT control byte into length to be read in TIOCPKT mode. Move post processing for OPOST to write process. Remove code related to variable "write_error". Return with EIO error if slave is already closed. (fhandler_pty_master::fhandler_pty_master): Remove initialization code for variable "need_nl". (fhandler_pty_common::process_opost_output): Add this function for handling of OPOST in write process. Add code to avoid blocking in non-blocking mode when output is suspended by ^S. (fhandler_pty_slave::write): Call fhandler_pty_common:: process_opost_output() instead of WriteFile(). Remove code related to variable "write_error". (fhandler_pty_master::doecho): Call fhandler_pty_common:: process_opost_output() instead of WriteFile(). * select.cc (peek_pipe): Remove code related to variable "need_nl". Signed-off-by: Corinna Vinschen --- winsup/cygwin/ChangeLog | 28 ++++ winsup/cygwin/fhandler.h | 5 +- winsup/cygwin/fhandler_tty.cc | 255 +++++++++++++++++----------------- winsup/cygwin/release/1.7.36 | 4 + winsup/cygwin/select.cc | 5 - winsup/cygwin/tty.cc | 1 + winsup/cygwin/tty.h | 2 +- 7 files changed, 168 insertions(+), 132 deletions(-) diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index a7fc6f9c4..46baa0433 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,31 @@ +2015-03-25 Takashi Yano + + * tty.h (class tty_min): Remove variable "write_error" to which any + errors are not currently set at anywhere. + (class tty): Add variable "column" for handling ONOCR. + * tty.cc (tty::init): Add initialization code for variable "column". + * fhandler.h (class fhandler_pty_master): Remove variable "need_nl" + which is not necessary any more. "need_nl" was needed by OPOST process + in fhandler_pty_master::process_slave_output(). + (class fhandler_pty_common): Add function process_opost_output() for + handling post processing for OPOST in write process. + * fhandler_tty.cc (fhandler_pty_master::process_slave_output): Count + TIOCPKT control byte into length to be read in TIOCPKT mode. Move + post processing for OPOST to write process. Remove code related to + variable "write_error". Return with EIO error if slave is already + closed. + (fhandler_pty_master::fhandler_pty_master): Remove initialization + code for variable "need_nl". + (fhandler_pty_common::process_opost_output): Add this function for + handling of OPOST in write process. Add code to avoid blocking in + non-blocking mode when output is suspended by ^S. + (fhandler_pty_slave::write): Call fhandler_pty_common:: + process_opost_output() instead of WriteFile(). Remove code related to + variable "write_error". + (fhandler_pty_master::doecho): Call fhandler_pty_common:: + process_opost_output() instead of WriteFile(). + * select.cc (peek_pipe): Remove code related to variable "need_nl". + 2015-03-24 Corinna Vinschen Per glibc BZ #15366: diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 4ec7d02fa..694c23bdd 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -1508,6 +1508,9 @@ class fhandler_pty_common: public fhandler_termios copyto (fh); return fh; } + + protected: + BOOL process_opost_output (HANDLE h, const void *ptr, ssize_t& len, bool is_echo); }; class fhandler_pty_slave: public fhandler_pty_common @@ -1574,8 +1577,6 @@ class fhandler_pty_master: public fhandler_pty_common DWORD dwProcessId; // Owner of master handles public: - int need_nl; // Next read should start with \n - HANDLE get_echo_handle () const { return echo_r; } /* Constructor */ fhandler_pty_master (int); diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc index 87bd6a0aa..89cc9e555 100644 --- a/winsup/cygwin/fhandler_tty.cc +++ b/winsup/cygwin/fhandler_tty.cc @@ -145,7 +145,8 @@ fhandler_pty_common::__release_output_mutex (const char *fn, int ln) void fhandler_pty_master::doecho (const void *str, DWORD len) { - if (!WriteFile (echo_w, str, len, &len, NULL)) + ssize_t towrite = len; + if (!process_opost_output (echo_w, str, towrite, true)) termios_printf ("Write to echo pipe failed, %E"); } @@ -217,10 +218,9 @@ int fhandler_pty_master::process_slave_output (char *buf, size_t len, int pktmode_on) { size_t rlen; - char outbuf[OUT_BUFFER_SIZE + 1]; + char outbuf[OUT_BUFFER_SIZE]; DWORD n; DWORD echo_cnt; - int column = 0; int rc = 0; flush_to_slave (); @@ -228,34 +228,8 @@ fhandler_pty_master::process_slave_output (char *buf, size_t len, int pktmode_on if (len == 0) goto out; - if (need_nl) - { - /* We need to return a left over \n character, resulting from - \r\n conversion. Note that we already checked for FLUSHO and - output_stopped at the time that we read the character, so we - don't check again here. */ - if (buf) - buf[0] = '\n'; - need_nl = 0; - rc = 1; - goto out; - } - for (;;) { - /* Set RLEN to the number of bytes to read from the pipe. */ - rlen = len; - if (get_ttyp ()->ti.c_oflag & OPOST && get_ttyp ()->ti.c_oflag & ONLCR) - { - /* We are going to expand \n to \r\n, so don't read more than - half of the number of bytes requested. */ - rlen /= 2; - if (rlen == 0) - rlen = 1; - } - if (rlen > sizeof outbuf) - rlen = sizeof outbuf; - n = echo_cnt = 0; for (;;) { @@ -267,7 +241,11 @@ fhandler_pty_master::process_slave_output (char *buf, size_t len, int pktmode_on if (n) break; if (hit_eof ()) - goto out; + { + set_errno (EIO); + rc = -1; + goto out; + } /* DISCARD (FLUSHO) and tcflush can finish here. */ if ((get_ttyp ()->ti.c_lflag & FLUSHO || !buf)) goto out; @@ -289,6 +267,26 @@ fhandler_pty_master::process_slave_output (char *buf, size_t len, int pktmode_on flush_to_slave (); } + /* Set RLEN to the number of bytes to read from the pipe. */ + rlen = len; + + char *optr; + optr = buf; + if (pktmode_on && buf) + { + *optr++ = TIOCPKT_DATA; + rlen -= 1; + } + + if (rlen == 0) + { + rc = optr - buf; + goto out; + } + + if (rlen > sizeof outbuf) + rlen = sizeof outbuf; + /* If echo pipe has data (something has been typed or pasted), prefer it over slave output. */ if (echo_cnt > 0) @@ -306,68 +304,12 @@ fhandler_pty_master::process_slave_output (char *buf, size_t len, int pktmode_on } termios_printf ("bytes read %u", n); - get_ttyp ()->write_error = 0; if (get_ttyp ()->ti.c_lflag & FLUSHO || !buf) continue; - char *optr; - optr = buf; - if (pktmode_on) - *optr++ = TIOCPKT_DATA; - - if (!(get_ttyp ()->ti.c_oflag & OPOST)) // post-process output - { - memcpy (optr, outbuf, n); - optr += n; - } - else // raw output mode - { - char *iptr = outbuf; - - while (n--) - { - switch (*iptr) - { - case '\r': - if ((get_ttyp ()->ti.c_oflag & ONOCR) && column == 0) - { - iptr++; - continue; - } - if (get_ttyp ()->ti.c_oflag & OCRNL) - *iptr = '\n'; - else - column = 0; - break; - case '\n': - if (get_ttyp ()->ti.c_oflag & ONLCR) - { - *optr++ = '\r'; - column = 0; - } - if (get_ttyp ()->ti.c_oflag & ONLRET) - column = 0; - break; - default: - column++; - break; - } - - /* Don't store data past the end of the user's buffer. This - can happen if the user requests a read of 1 byte when - doing \r\n expansion. */ - if (optr - buf >= (int) len) - { - if (*iptr != '\n' || n != 0) - system_printf ("internal error: %u unexpected characters", n); - need_nl = 1; - break; - } - - *optr++ = *iptr++; - } - } + memcpy (optr, outbuf, n); + optr += n; rc = optr - buf; break; @@ -639,7 +581,6 @@ fhandler_pty_slave::init (HANDLE h, DWORD a, mode_t) ssize_t __stdcall fhandler_pty_slave::write (const void *ptr, size_t len) { - DWORD n; ssize_t towrite = len; bg_check_types bg = bg_check (SIGTTOU); @@ -650,43 +591,19 @@ fhandler_pty_slave::write (const void *ptr, size_t len) push_process_state process_state (PID_TTYOU); - while (len) + if (!process_opost_output (get_output_handle (), ptr, towrite, false)) { - n = MIN (OUT_BUFFER_SIZE, len); - char *buf = (char *)ptr; - ptr = (char *) ptr + n; - len -= n; - - while (tc ()->output_stopped) - cygwait (10); - - /* Previous write may have set write_error to != 0. Check it here. - This is less than optimal, but the alternative slows down pty - writes enormously. */ - if (get_ttyp ()->write_error) + DWORD err = GetLastError (); + termios_printf ("WriteFile failed, %E"); + switch (err) { - set_errno (get_ttyp ()->write_error); - towrite = -1; - get_ttyp ()->write_error = 0; - break; - } - - BOOL res = WriteFile (get_output_handle (), buf, n, &n, NULL); - if (!res) - { - DWORD err = GetLastError (); - termios_printf ("WriteFile failed, %E"); - switch (err) - { - case ERROR_NO_DATA: - err = ERROR_IO_DEVICE; - default: - __seterrno_from_win_error (err); - } - raise (SIGHUP); /* FIXME: Should this be SIGTTOU? */ - towrite = -1; - break; + case ERROR_NO_DATA: + err = ERROR_IO_DEVICE; + default: + __seterrno_from_win_error (err); } + raise (SIGHUP); /* FIXME: Should this be SIGTTOU? */ + towrite = -1; } return towrite; } @@ -1225,7 +1142,7 @@ errout: fhandler_pty_master::fhandler_pty_master (int unit) : fhandler_pty_common (), pktmode (0), master_ctl (NULL), master_thread (NULL), from_master (NULL), to_master (NULL), - echo_r (NULL), echo_w (NULL), dwProcessId (0), need_nl (0) + echo_r (NULL), echo_w (NULL), dwProcessId (0) { if (unit >= 0) dev ().parse (DEV_PTYM_MAJOR, unit); @@ -1783,3 +1700,93 @@ fhandler_pty_master::fixup_after_exec () else from_master = to_master = NULL; } + +BOOL +fhandler_pty_common::process_opost_output (HANDLE h, const void *ptr, ssize_t& len, bool is_echo) +{ + ssize_t towrite = len; + BOOL res = TRUE; + while (towrite) + { + if (!is_echo) + { + if (tc ()->output_stopped && is_nonblocking ()) + { + if (towrite < len) + break; + else + { + set_errno(EAGAIN); + len = -1; + return TRUE; + } + } + while (tc ()->output_stopped) + cygwait (10); + } + + if (!(get_ttyp ()->ti.c_oflag & OPOST)) // raw output mode + { + DWORD n = MIN (OUT_BUFFER_SIZE, towrite); + res = WriteFile (h, ptr, n, &n, NULL); + if (!res) + break; + ptr = (char *) ptr + n; + towrite -= n; + } + else // post-process output + { + char outbuf[OUT_BUFFER_SIZE + 1]; + char *buf = (char *)ptr; + DWORD n = 0; + ssize_t rc = 0; + acquire_output_mutex (INFINITE); + while (n < OUT_BUFFER_SIZE && rc < towrite) + { + switch (buf[rc]) + { + case '\r': + if ((get_ttyp ()->ti.c_oflag & ONOCR) + && get_ttyp ()->column == 0) + { + rc++; + continue; + } + if (get_ttyp ()->ti.c_oflag & OCRNL) + { + outbuf[n++] = '\n'; + rc++; + } + else + { + outbuf[n++] = buf[rc++]; + get_ttyp ()->column = 0; + } + break; + case '\n': + if (get_ttyp ()->ti.c_oflag & ONLCR) + { + outbuf[n++] = '\r'; + get_ttyp ()->column = 0; + } + if (get_ttyp ()->ti.c_oflag & ONLRET) + get_ttyp ()->column = 0; + outbuf[n++] = buf[rc++]; + break; + default: + outbuf[n++] = buf[rc++]; + get_ttyp ()->column++; + break; + } + } + release_output_mutex (); + res = WriteFile (h, outbuf, n, &n, NULL); + if (!res) + break; + ptr = (char *) ptr + rc; + towrite -= rc; + } + } + len -= towrite; + return res; +} diff --git a/winsup/cygwin/release/1.7.36 b/winsup/cygwin/release/1.7.36 index 28797779a..e9cfc6154 100644 --- a/winsup/cygwin/release/1.7.36 +++ b/winsup/cygwin/release/1.7.36 @@ -18,3 +18,7 @@ Bug Fixes - Fix a name change from symlink to target name in calls to execvp, system, etc. Addresses: https://cygwin.com/ml/cygwin/2015-03/msg00270.html + +- Fix internal error in pty -ONLCR handling. Fix timing bug in pty OPOST + handling. + Addresses: https://cygwin.com/ml/cygwin/2015-02/msg00929.html diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc index 17a32a367..1706c87c8 100644 --- a/winsup/cygwin/select.cc +++ b/winsup/cygwin/select.cc @@ -604,11 +604,6 @@ peek_pipe (select_record *s, bool from_select) { fhandler_pty_master *fhm = (fhandler_pty_master *) fh; fhm->flush_to_slave (); - if (fhm->need_nl) - { - gotone = s->read_ready = true; - goto out; - } } break; default: diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc index 01c53f933..7de0fa91c 100644 --- a/winsup/cygwin/tty.cc +++ b/winsup/cygwin/tty.cc @@ -237,6 +237,7 @@ tty::init () was_opened = false; master_pid = 0; is_console = false; + column = 0; } HANDLE diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h index c52f26307..9790a365a 100644 --- a/winsup/cygwin/tty.h +++ b/winsup/cygwin/tty.h @@ -67,7 +67,6 @@ public: * -ERRNO */ int ioctl_retval; - int write_error; void setntty (_major_t t, _minor_t n) {ntty = (fh_devices) FHDEV (t, n);} dev_t getntty () const {return ntty;} @@ -117,6 +116,7 @@ public: int read_retval; bool was_opened; /* True if opened at least once. */ + int column; /* Current Column */ void init (); HANDLE open_inuse (ACCESS_MASK access);