[newlib-cygwin] Cygwin: FIFO: code simplification

[newlib-cygwin] Cygwin: FIFO: code simplification

[Ken Brown]

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=1f273459473e8a5a7e8b32da8e4b88c16841bd8c

commit 1f273459473e8a5a7e8b32da8e4b88c16841bd8c
Author: Ken Brown <kbrown@cornell.edu>
Date:   Sat May 9 17:25:39 2020 -0400

    Cygwin: FIFO: code simplification
    
    There are currently three functions that call NtQueryInformationFile
    to determine the state of a pipe instance.  Do this only once, in a
    new fifo_client_handler::set_state () function, and call that when
    state information is needed.
    
    Remove the fifo_client_handler methods pipe_state and get_state, which
    are no longer needed.
    
    Make fhandler_fifo::get_fc_handler return a reference, for use in
    select.cc:peek_fifo.
    
    Make other small changes to ensure that this commit doesn't change any
    decisions based on the state of a fifo_client_handler.
    
    The tricky part is interpreting FILE_PIPE_CLOSING_STATE, which we
    translate to fc_closing.  Our current interpretation, which is not
    changing as a result of this commit, is that the writer at the other
    end of the pipe instance is viewed as still connected from the point
    of view of raw_read and determining EOF.
    
    But it is not viewed as still connected if we are deciding whether to
    unblock a new reader that is trying to open.

Diff:
---
 winsup/cygwin/fhandler.h       | 23 +++++++-------
 winsup/cygwin/fhandler_fifo.cc | 68 ++++++++++++++++++++----------------------
 winsup/cygwin/select.cc        | 20 ++-----------
 3 files changed, 44 insertions(+), 67 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9a82d491a..ae64086df 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1269,34 +1269,31 @@ public:
 
 #define CYGWIN_FIFO_PIPE_NAME_LEN     47
 
-/* The last three are the ones we try to read from. */
+/* We view the fc_closing state as borderline between open and closed
+   for a writer at the other end of a fifo_client_handler.
+
+   We still attempt to read from the writer when the handler is in
+   this state, and we don't declare a reader to be at EOF if there's a
+   handler in this state.  But the existence of a handler in this
+   state is not sufficient to unblock a reader trying to open. */
 enum fifo_client_connect_state
 {
   fc_unknown,
   fc_error,
   fc_disconnected,
   fc_listening,
-  fc_connected,
   fc_closing,
+  fc_connected,
   fc_input_avail,
 };
 
-enum
-{
-  FILE_PIPE_INPUT_AVAILABLE_STATE = 5
-};
-
 struct fifo_client_handler
 {
   HANDLE h;
   fifo_client_connect_state state;
   fifo_client_handler () : h (NULL), state (fc_unknown) {}
   void close () { NtClose (h); }
-/* Returns FILE_PIPE_DISCONNECTED_STATE, FILE_PIPE_LISTENING_STATE,
-   FILE_PIPE_CONNECTED_STATE, FILE_PIPE_CLOSING_STATE,
-   FILE_PIPE_INPUT_AVAILABLE_STATE, or -1 on error. */
-  fifo_client_connect_state &get_state () { return state; }
-  int pipe_state ();
+  fifo_client_connect_state set_state ();
 };
 
 class fhandler_fifo;
@@ -1462,7 +1459,7 @@ public:
   bool maybe_eof () const { return _maybe_eof; }
   void maybe_eof (bool val) { _maybe_eof = val; }
   int get_nhandlers () const { return nhandlers; }
-  fifo_client_handler get_fc_handler (int i) const { return fc_handler[i]; }
+  fifo_client_handler &get_fc_handler (int i) { return fc_handler[i]; }
   PUNICODE_STRING get_pipe_name ();
   DWORD fifo_reader_thread_func ();
   void fifo_client_lock () { _fifo_client_lock.lock (); }
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index ec7c5d427..9cc00d5e7 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -343,7 +343,7 @@ fhandler_fifo::delete_client_handler (int i)
 	     (nhandlers - i) * sizeof (fc_handler[i]));
 }
 
-/* Delete invalid handlers. */
+/* Delete handlers that we will never read from. */
 void
 fhandler_fifo::cleanup_handlers ()
 {
@@ -351,7 +351,7 @@ fhandler_fifo::cleanup_handlers ()
 
   while (i < nhandlers)
     {
-      if (fc_handler[i].state < fc_connected)
+      if (fc_handler[i].state < fc_closing)
 	delete_client_handler (i);
       else
 	i++;
@@ -417,9 +417,6 @@ fhandler_fifo::update_my_handlers (bool from_exec)
 
       for (int i = 0; i < get_shared_nhandlers (); i++)
 	{
-	  /* Should never happen. */
-	  if (shared_fc_handler[i].state < fc_connected)
-	    continue;
 	  if (add_client_handler (false) < 0)
 	    api_fatal ("Can't add client handler, %E");
 	  fifo_client_handler &fc = fc_handler[nhandlers - 1];
@@ -462,30 +459,9 @@ fhandler_fifo::check_write_ready ()
 {
   bool set = false;
 
-  fifo_client_lock ();
   for (int i = 0; i < nhandlers && !set; i++)
-    switch (fc_handler[i].pipe_state ())
-      {
-      case FILE_PIPE_CONNECTED_STATE:
-	fc_handler[i].state = fc_connected;
-	set = true;
-	break;
-      case FILE_PIPE_INPUT_AVAILABLE_STATE:
-	fc_handler[i].state = fc_input_avail;
-	set = true;
-	break;
-      case FILE_PIPE_DISCONNECTED_STATE:
-	fc_handler[i].state = fc_disconnected;
-	break;
-      case FILE_PIPE_LISTENING_STATE:
-	fc_handler[i].state = fc_listening;
-      case FILE_PIPE_CLOSING_STATE:
-	fc_handler[i].state = fc_closing;
-      default:
-	fc_handler[i].state = fc_error;
-	break;
-      }
-  fifo_client_unlock ();
+    if (fc_handler[i].set_state () >= fc_connected)
+      set = true;
   if (set || IsEventSignalled (writer_opening))
     SetEvent (write_ready);
   else
@@ -656,13 +632,13 @@ fhandler_fifo::fifo_reader_thread_func ()
 	    default:
 	      break;
 	    }
-	  fifo_client_unlock ();
 	  if (ph)
 	    NtClose (ph);
 	  if (update && update_shared_handlers () < 0)
 	    api_fatal ("Can't update shared handlers, %E");
 	  if (check)
 	    check_write_ready ();
+	  fifo_client_unlock ();
 	  if (cancel)
 	    goto canceled;
 	}
@@ -1307,7 +1283,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
       int nconnected = 0;
       fifo_client_lock ();
       for (int i = 0; i < nhandlers; i++)
-	if (fc_handler[i].state >= fc_connected)
+	if (fc_handler[i].state >= fc_closing)
 	  {
 	    NTSTATUS status;
 	    IO_STATUS_BLOCK io;
@@ -1411,25 +1387,45 @@ fhandler_fifo::close_all_handlers ()
   nhandlers = 0;
 }
 
-int
-fifo_client_handler::pipe_state ()
+fifo_client_connect_state
+fifo_client_handler::set_state ()
 {
   IO_STATUS_BLOCK io;
   FILE_PIPE_LOCAL_INFORMATION fpli;
   NTSTATUS status;
 
+  if (!h)
+    return (state = fc_unknown);
+
   status = NtQueryInformationFile (h, &io, &fpli,
 				   sizeof (fpli), FilePipeLocalInformation);
   if (!NT_SUCCESS (status))
     {
       debug_printf ("NtQueryInformationFile status %y", status);
-      __seterrno_from_nt_status (status);
-      return -1;
+      state = fc_error;
     }
   else if (fpli.ReadDataAvailable > 0)
-    return FILE_PIPE_INPUT_AVAILABLE_STATE;
+    state = fc_input_avail;
   else
-    return fpli.NamedPipeState;
+    switch (fpli.NamedPipeState)
+      {
+      case FILE_PIPE_DISCONNECTED_STATE:
+	state = fc_disconnected;
+	break;
+      case FILE_PIPE_LISTENING_STATE:
+	state = fc_listening;
+	break;
+      case FILE_PIPE_CONNECTED_STATE:
+	state = fc_connected;
+	break;
+      case FILE_PIPE_CLOSING_STATE:
+	state = fc_closing;
+	break;
+      default:
+	state = fc_error;
+	break;
+      }
+  return state;
 }
 
 void
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 2c299acf7..9ae567c38 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -871,32 +871,16 @@ peek_fifo (select_record *s, bool from_select)
       fh->fifo_client_lock ();
       int nconnected = 0;
       for (int i = 0; i < fh->get_nhandlers (); i++)
-	if (fh->get_fc_handler (i).get_state () >= fc_connected)
+	if (fh->get_fc_handler (i).set_state () >= fc_closing)
 	  {
 	    nconnected++;
-	    switch (fh->get_fc_handler (i).pipe_state ())
+	    if (fh->get_fc_handler (i).state == fc_input_avail)
 	      {
-	      case FILE_PIPE_CONNECTED_STATE:
-		fh->get_fc_handler (i).get_state () = fc_connected;
-		break;
-	      case FILE_PIPE_DISCONNECTED_STATE:
-		fh->get_fc_handler (i).get_state () = fc_disconnected;
-		nconnected--;
-		break;
-	      case FILE_PIPE_CLOSING_STATE:
-		fh->get_fc_handler (i).get_state () = fc_closing;
-		break;
-	      case FILE_PIPE_INPUT_AVAILABLE_STATE:
-		fh->get_fc_handler (i).get_state () = fc_input_avail;
 		select_printf ("read: %s, ready for read", fh->get_name ());
 		fh->fifo_client_unlock ();
 		fh->reading_unlock ();
 		gotone += s->read_ready = true;
 		goto out;
-	      default:
-		fh->get_fc_handler (i).get_state () = fc_error;
-		nconnected--;
-		break;
 	      }
 	  }
       fh->maybe_eof (!nconnected);