From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2210) id 3203C38930CA; Mon, 11 May 2020 13:55:42 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3203C38930CA Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: Ken Brown To: cygwin-cvs@sourceware.org Subject: [newlib-cygwin] Cygwin: FIFO: code simplification X-Act-Checkin: newlib-cygwin X-Git-Author: Ken Brown X-Git-Refname: refs/heads/master X-Git-Oldrev: 2125ca8a69d0680ffe8079c15ef479f869ee35cc X-Git-Newrev: 1f273459473e8a5a7e8b32da8e4b88c16841bd8c Message-Id: <20200511135542.3203C38930CA@sourceware.org> Date: Mon, 11 May 2020 13:55:42 +0000 (GMT) X-BeenThere: cygwin-cvs@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin core component git logs List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 May 2020 13:55:42 -0000 https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=1f273459473e8a5a7e8b32da8e4b88c16841bd8c commit 1f273459473e8a5a7e8b32da8e4b88c16841bd8c Author: Ken Brown 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);