Cygwin: serial: revamp overlapped IO in read and select

Get rid of WaitCommEvent and using overlapped_armed to share the
same overlapped operation between read and select.  Rather, make
sure to cancel the overlapped IO before leaving any of these functions.

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>

winsup/cygwin/fhandler.h

@@ -1687,7 +1687,6 @@ class fhandler_serial: public fhandler_base
   int dtr;				/* for Windows 9x purposes only */
 
  public:
-  int overlapped_armed;
   OVERLAPPED io_status;
 
   /* Constructor */

winsup/cygwin/fhandler_serial.cc

@@ -35,15 +35,14 @@ fhandler_serial::overlapped_setup ()
   memset (&io_status, 0, sizeof (io_status));
   io_status.hEvent = CreateEvent (&sec_none_nih, TRUE, FALSE, NULL);
   ProtectHandle (io_status.hEvent);
-  overlapped_armed = 0;
 }
 
 void __reg3
 fhandler_serial::raw_read (void *ptr, size_t& ulen)
 {
-  DWORD io_err, event;
+  DWORD io_err;
   COMSTAT st;
-  DWORD bytes_to_read, read_bytes, undefined;
+  DWORD bytes_to_read, read_bytes;
   ssize_t tot = 0;
 
   if (ulen > SSIZE_MAX)
@@ -57,11 +56,6 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen)
 
   debug_printf ("ulen %ld, vmin_ %u, vtime_ %u, hEvent %p",
 		ulen, vmin_, vtime_, io_status.hEvent);
-  if (!overlapped_armed)
-    {
-      SetCommMask (get_handle (), EV_RXCHAR);
-      ResetEvent (io_status.hEvent);
-    }
 
   do
     {
@@ -93,58 +87,8 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen)
 	     and don't wait. */
 	  if (st.cbInQue && st.cbInQue >= minchars)
 	    bytes_to_read = MIN (st.cbInQue, bytes_to_read);
-	  /* if vtime_ is set, use kernel timeouts, otherwise wait here. */
-	  else if (vtime_ == 0 && !overlapped_armed)
-	    {
-	      if (WaitCommEvent (get_handle (), &event, &io_status))
-		{
-		  debug_printf ("WaitCommEvent succeeded: event %x", event);
-		  if (!event)
-		    continue;
-		}
-	      else if (GetLastError () != ERROR_IO_PENDING)
-		goto err;
-	      overlapped_armed = 1;
-	    }
-	}
-      /* overlapped_armed may be set by select, so we have to make sure
-	 to disarm even in O_NONBLOCK mode. */
-      if (overlapped_armed)
-	{
-	  switch (cygwait (io_status.hEvent, is_nonblocking () ? 0 : INFINITE))
-	    {
-	    case WAIT_OBJECT_0:
-	      if (!GetOverlappedResult (get_handle (), &io_status,
-					&undefined, TRUE))
-		goto err;
-	      debug_printf ("overlapped event %x", event);
-	      break;
-	    case WAIT_SIGNALED:
-	      CancelIo (get_handle ());
-	      overlapped_armed = 0;
-	      if (!GetOverlappedResult (get_handle (), &io_status,
-					&undefined, TRUE))
-		goto err;
-	      /* Only if no bytes read, return with EINTR. */
-	      if (!tot)
-		{
-		  tot = -1;
-		  set_sig_errno (EINTR);
-		}
-	      goto out;
-	    case WAIT_CANCELED:
-	      CancelIo (get_handle ());
-	      overlapped_armed = 0;
-	      GetOverlappedResult (get_handle (), &io_status, &undefined,
-				   TRUE);
-	      pthread::static_cancel_self ();
-	      /*NOTREACHED*/
-	    default:
-	      goto err;
-	    }
 	}
 
-      overlapped_armed = 0;
       ResetEvent (io_status.hEvent);
       if (!ReadFile (get_handle (), ptr, bytes_to_read, &read_bytes,
 		     &io_status))
@@ -152,11 +96,51 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen)
 	  if (GetLastError () != ERROR_IO_PENDING)
 	    goto err;
 	  if (is_nonblocking ())
+	    {
 	      CancelIo (get_handle ());
 	      if (!GetOverlappedResult (get_handle (), &io_status, &read_bytes,
 					TRUE))
 		goto err;
 	    }
+	  else
+	    {
+	      switch (cygwait (io_status.hEvent))
+		{
+		default: /* Handle an error case from cygwait basically like
+			    a cancel condition and see if we got "something" */
+		  CancelIo (get_handle ());
+		  /*FALLTHRU*/
+		case WAIT_OBJECT_0:
+		  if (!GetOverlappedResult (get_handle (), &io_status,
+					    &read_bytes, TRUE))
+		    goto err;
+		  debug_printf ("got %u bytes from ReadFile", read_bytes);
+		  break;
+		case WAIT_SIGNALED:
+		  CancelIo (get_handle ());
+		  if (!GetOverlappedResult (get_handle (), &io_status,
+					    &read_bytes, TRUE))
+		    goto err;
+		  /* Only if no bytes read, return with EINTR. */
+		  if (!tot && !read_bytes)
+		    {
+		      tot = -1;
+		      set_sig_errno (EINTR);
+		      debug_printf ("signal received, set EINTR");
+		    }
+		  else
+		    debug_printf ("signal received but ignored");
+		  goto out;
+		case WAIT_CANCELED:
+		  CancelIo (get_handle ());
+		  GetOverlappedResult (get_handle (), &io_status, &read_bytes,
+				       TRUE);
+		  debug_printf ("thread canceled");
+		  pthread::static_cancel_self ();
+		  /*NOTREACHED*/
+		}
+	    }
+	}
       tot += read_bytes;
       ptr = (void *) ((caddr_t) ptr + read_bytes);
       ulen -= read_bytes;
@@ -174,9 +158,6 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen)
 	      tot = -1;
 	      __seterrno ();
 	    }
-	  CancelIo (get_handle ());
-	  overlapped_armed = 0;
-	  GetOverlappedResult (get_handle (), &io_status, &undefined, TRUE);
 	  break;
 	}
     }

winsup/cygwin/select.cc

@@ -1434,22 +1434,15 @@ fhandler_dev_null::select_except (select_stuff *ss)
   return s;
 }
 
-static int start_thread_serial (select_record *me, select_stuff *stuff);
-
 static int
 peek_serial (select_record *s, bool)
 {
+  HANDLE h;
   COMSTAT st;
-  DWORD event, io_err;
+  DWORD io_err;
 
   fhandler_serial *fh = (fhandler_serial *) s->fh;
 
-  if (fh->get_readahead_valid ())
-    return s->read_ready = true;
-
-  select_printf ("fh->overlapped_armed %d", fh->overlapped_armed);
-
-  HANDLE h;
   set_handle_or_return_if_not_open (h, s);
 
   if ((s->read_selected && s->read_ready)
@@ -1459,17 +1452,8 @@ peek_serial (select_record *s, bool)
       return true;
     }
 
-  /* This is apparently necessary for the com0com driver.
+  if (fh->get_readahead_valid ())
-     See: http://cygwin.com/ml/cygwin/2009-01/msg00667.html */
+    return s->read_ready = true;
-  SetCommMask (h, 0);
-
-  SetCommMask (h, EV_RXCHAR);
-
-  if (!fh->overlapped_armed)
-    {
-      COMSTAT st;
-
-      ResetEvent (fh->io_status.hEvent);
 
   if (!ClearCommError (h, &io_err, &st))
     {
@@ -1478,36 +1462,6 @@ peek_serial (select_record *s, bool)
     }
   if (st.cbInQue)
     return s->read_ready = true;
-      if (WaitCommEvent (h, &event, &fh->io_status))
-	return s->read_ready = true;
-      if (GetLastError () != ERROR_IO_PENDING)
-	{
-	  debug_printf ("WaitCommEvent %E");
-	  goto err;
-	}
-      fh->overlapped_armed = 1;
-    }
-
-  switch (WaitForSingleObject (fh->io_status.hEvent, 10L))
-    {
-    case WAIT_OBJECT_0:
-      if (!ClearCommError (h, &io_err, &st))
-	{
-	  debug_printf ("ClearCommError %E");
-	  goto err;
-	}
-      if (st.cbInQue)
-	{
-	  select_printf ("got something");
-	  return s->read_ready = true;
-	}
-      break;
-    case WAIT_TIMEOUT:
-      break;
-    default:
-      debug_printf ("WaitForMultipleObjects %E");
-      goto err;
-    }
 
   return 0;
 
@@ -1515,84 +1469,49 @@ err:
   if (GetLastError () == ERROR_OPERATION_ABORTED)
     {
       select_printf ("operation aborted");
-      return 0;
+      return false;
     }
 
   s->set_select_errno ();
   return -1;
 }
 
-static DWORD WINAPI
-thread_serial (void *arg)
-{
-  select_serial_info *si = (select_serial_info *) arg;
-  bool looping = true;
-
-  while (looping)
-    for (select_record *s = si->start; (s = s->next); )
-      if (s->startup != start_thread_serial)
-	continue;
-      else
-	{
-	  if (peek_serial (s, true))
-	    looping = false;
-	  if (si->stop_thread)
-	    {
-	      select_printf ("stopping");
-	      looping = false;
-	      break;
-	    }
-	}
-
-  select_printf ("exiting");
-  return 0;
-}
-
-static int
-start_thread_serial (select_record *me, select_stuff *stuff)
-{
-  if (stuff->device_specific_serial)
-    me->h = *((select_serial_info *) stuff->device_specific_serial)->thread;
-  else
-    {
-      select_serial_info *si = new select_serial_info;
-      si->start = &stuff->start;
-      si->stop_thread = false;
-      si->thread = new cygthread (thread_serial, si, "sersel");
-      me->h = *si->thread;
-      stuff->device_specific_serial = si;
-    }
-  return 1;
-}
-
 static void
-serial_cleanup (select_record *, select_stuff *stuff)
+serial_read_cleanup (select_record *s, select_stuff *stuff)
 {
-  select_serial_info *si = (select_serial_info *) stuff->device_specific_serial;
+  if (s->h)
-  if (!si)
-    return;
-  if (si->thread)
     {
-      si->stop_thread = true;
+      fhandler_serial *fh = (fhandler_serial *) s->fh;
-      si->thread->detach ();
+      HANDLE h = fh->get_handle_cyg ();
+      DWORD undefined;
+
+      if (h)
+	{
+	  CancelIo (h);
+	  GetOverlappedResult (h, &fh->io_status, &undefined, TRUE);
+	}
     }
-  delete si;
-  stuff->device_specific_serial = NULL;
 }
 
 select_record *
 fhandler_serial::select_read (select_stuff *ss)
 {
+  DWORD event;
   select_record *s = ss->start.next;
-  if (!s->startup)
+
-    {
+  s->startup = no_startup;
-      s->startup = start_thread_serial;
   s->verify = verify_ok;
-      s->cleanup = serial_cleanup;
+  s->cleanup = serial_read_cleanup;
-    }
   s->peek = peek_serial;
   s->read_selected = true;
   s->read_ready = false;
+  /* This is apparently necessary for the com0com driver.
+     See: http://cygwin.com/ml/cygwin/2009-01/msg00667.html */
+  SetCommMask (get_handle_cyg (), 0);
+  SetCommMask (get_handle_cyg (), EV_RXCHAR);
+  if (!WaitCommEvent (get_handle_cyg (), &event, &io_status)
+      && GetLastError () == ERROR_IO_PENDING)
+    s->h = io_status.hEvent;
   return s;
 }
 
@@ -1600,13 +1519,10 @@ select_record *
 fhandler_serial::select_write (select_stuff *ss)
 {
   select_record *s = ss->start.next;
-  if (!s->startup)
+
-    {
   s->startup = no_startup;
   s->verify = verify_ok;
-    }
   s->peek = peek_serial;
-  s->h = get_handle ();
   s->write_selected = true;
   s->write_ready = true;
   return s;
@@ -1616,12 +1532,9 @@ select_record *
 fhandler_serial::select_except (select_stuff *ss)
 {
   select_record *s = ss->start.next;
-  if (!s->startup)
+
-    {
   s->startup = no_startup;
   s->verify = verify_ok;
-    }
-  s->h = NULL;
   s->peek = peek_serial;
   s->except_selected = false;	// Can't do this
   s->except_ready = false;

winsup/cygwin/select.h

@@ -67,11 +67,6 @@ struct select_socket_info: public select_info
   select_socket_info (): select_info (), num_w4 (0), ser_num (0), w4 (NULL) {}
 };
 
-struct select_serial_info: public select_info
-{
-  select_serial_info (): select_info () {}
-};
-
 class select_stuff
 {
 public:
@@ -92,7 +87,6 @@ public:
   select_pipe_info *device_specific_ptys;
   select_fifo_info *device_specific_fifo;
   select_socket_info *device_specific_socket;
-  select_serial_info *device_specific_serial;
 
   bool test_and_set (int, fd_set *, fd_set *, fd_set *);
   int poll (fd_set *, fd_set *, fd_set *);
@@ -105,8 +99,7 @@ public:
 		   device_specific_pipe (NULL),
 		   device_specific_ptys (NULL),
 		   device_specific_fifo (NULL),
-		   device_specific_socket (NULL),
+		   device_specific_socket (NULL)
-		   device_specific_serial (NULL)
 		   {}
 };