Re: TIOCPKT mode of PTY is broken if ONLCR bit is cleared.

[Takashi Yano <takashi.yano@nifty.ne.jp>]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Sat, 28 Feb 2015 14:59:47 +0100
Corinna Vinschen <corinna-cygwin@cygwin.com> wrote:

> As soon as we have your CA, we can discuss this patch further.

Can I send CA form by e-mail? Or should I send it as a postal mail?

> For the time being, your patch is lacking a ChangeLog entry.  I scanned
> your patch a bit, and it looks pretty good, but I'll need some more
> explanations and the patch should definitely be made against current
> CVS (soon to be git), not the latest release version.

Thank you for advice. I have revised the patch to follow your direction.
Please find new patch attached.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: cygwin.patch.20150302 --]
[-- Type: application/octet-stream, Size: 12750 bytes --]

Index: src/winsup/cygwin/ChangeLog
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/ChangeLog,v
retrieving revision 1.6666
diff -u -p -r1.6666 ChangeLog
--- src/winsup/cygwin/ChangeLog	2 Mar 2015 10:52:07 -0000	1.6666
+++ src/winsup/cygwin/ChangeLog	2 Mar 2015 10:59:22 -0000
@@ -1,3 +1,31 @@
+2015-03-02  Takashi Yano <takashi.yano@nifty.ne.jp>
+
+	* tty.h (class tty_min): Add variable "column" for handling ONOCR.
+	Remove variable "write_error" to which any errors are not currently
+	set at anywhere.
+	* 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-02  Corinna Vinschen  <corinna@vinschen.de>
 
 	* security.cc (get_attribute_from_acl): Don't spill Everyone permissions
Index: src/winsup/cygwin/fhandler.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/fhandler.h,v
retrieving revision 1.513
diff -u -p -r1.513 fhandler.h
--- src/winsup/cygwin/fhandler.h	24 Feb 2015 11:05:02 -0000	1.513
+++ src/winsup/cygwin/fhandler.h	2 Mar 2015 10:59:23 -0000
@@ -1507,6 +1507,9 @@ class fhandler_pty_common: public fhandl
     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
@@ -1572,8 +1575,6 @@ class fhandler_pty_master: public fhandl
   DWORD dwProcessId;		// Owner of master handles
 
 public:
-  int need_nl;			// Next read should start with \n
-
   /* Constructor */
   fhandler_pty_master (int);
 
Index: src/winsup/cygwin/fhandler_tty.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/fhandler_tty.cc,v
retrieving revision 1.290
diff -u -p -r1.290 fhandler_tty.cc
--- src/winsup/cygwin/fhandler_tty.cc	25 Feb 2015 16:46:57 -0000	1.290
+++ src/winsup/cygwin/fhandler_tty.cc	2 Mar 2015 10:59:23 -0000
@@ -145,10 +145,9 @@ fhandler_pty_common::__release_output_mu
 void
 fhandler_pty_master::doecho (const void *str, DWORD len)
 {
-  acquire_output_mutex (INFINITE);
-  if (!WriteFile (to_master, str, len, &len, NULL))
+  ssize_t towrite = len;
+  if (!process_opost_output (to_master, str, towrite, true))
     termios_printf ("Write to %p failed, %E", to_master);
-  release_output_mutex ();
 }
 
 int
@@ -219,9 +218,8 @@ 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;
-  int column = 0;
   int rc = 0;
 
   flush_to_slave ();
@@ -229,34 +227,8 @@ fhandler_pty_master::process_slave_outpu
   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 = 0;
       for (;;)
 	{
@@ -265,7 +237,11 @@ fhandler_pty_master::process_slave_outpu
 	  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;
@@ -287,6 +263,26 @@ fhandler_pty_master::process_slave_outpu
 	  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 (!ReadFile (get_handle (), outbuf, rlen, &n, NULL))
 	{
 	  termios_printf ("ReadFile failed, %E");
@@ -294,68 +290,12 @@ fhandler_pty_master::process_slave_outpu
 	}
 
       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;
 
@@ -633,7 +573,6 @@ fhandler_pty_slave::init (HANDLE h, DWOR
 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);
@@ -644,46 +583,19 @@ fhandler_pty_slave::write (const void *p
 
   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);
-      acquire_output_mutex (INFINITE);
-
-      /* 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)
-	{
-	  set_errno (get_ttyp ()->write_error);
-	  towrite = -1;
-	  get_ttyp ()->write_error = 0;
-	  release_output_mutex ();
-	  break;
-	}
-
-      BOOL res = WriteFile (get_output_handle (), buf, n, &n, NULL);
-      release_output_mutex ();
-      if (!res)
+      DWORD err = GetLastError ();
+      termios_printf ("WriteFile failed, %E");
+      switch (err)
 	{
-	  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;
 }
@@ -1228,7 +1140,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),
-    dwProcessId (0), need_nl (0)
+    dwProcessId (0)
 {
   if (unit >= 0)
     dev ().parse (DEV_PTYM_MAJOR, unit);
@@ -1763,3 +1675,94 @@ 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);
+	}
+
+      acquire_output_mutex (INFINITE);
+
+      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;
+	  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;
+		}
+	    }
+	  res = WriteFile (h, outbuf, n, &n, NULL);
+	  if (!res)
+	    break;
+	  ptr = (char *) ptr + rc;
+	  towrite -= rc;
+	}
+
+      release_output_mutex ();
+    }
+  len -= towrite;
+  return res;
+}
Index: src/winsup/cygwin/select.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/select.cc,v
retrieving revision 1.223
diff -u -p -r1.223 select.cc
--- src/winsup/cygwin/select.cc	11 Oct 2014 12:14:29 -0000	1.223
+++ src/winsup/cygwin/select.cc	2 Mar 2015 10:59:23 -0000
@@ -604,11 +604,6 @@ peek_pipe (select_record *s, bool from_s
 	  {
 	    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:
Index: src/winsup/cygwin/tty.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/tty.cc,v
retrieving revision 1.96
diff -u -p -r1.96 tty.cc
--- src/winsup/cygwin/tty.cc	18 Oct 2013 20:07:35 -0000	1.96
+++ src/winsup/cygwin/tty.cc	2 Mar 2015 10:59:23 -0000
@@ -237,6 +237,7 @@ tty::init ()
   was_opened = false;
   master_pid = 0;
   is_console = false;
+  column = 0;
 }
 
 HANDLE
Index: src/winsup/cygwin/tty.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/tty.h,v
retrieving revision 1.37
diff -u -p -r1.37 tty.h
--- src/winsup/cygwin/tty.h	23 Apr 2013 09:44:34 -0000	1.37
+++ src/winsup/cygwin/tty.h	2 Mar 2015 10:59:23 -0000
@@ -67,7 +67,8 @@ public:
    * -ERRNO
    */
   int ioctl_retval;
-  int write_error;
+
+  int column;	/* Current Column */
 
   void setntty (_major_t t, _minor_t n) {ntty = (fh_devices) FHDEV (t, n);}
   dev_t getntty () const {return ntty;}

[-- Attachment #3: Type: text/plain, Size: 218 bytes --]

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple