[PATCH v4.2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
Takashi Yano
takashi.yano@nifty.ne.jp
Tue Sep 17 13:49:01 GMT 2024
Hi Ken,
Thanks for reviewing!
On Sat, 14 Sep 2024 16:09:19 -0400
Ken Brown wrote:
> On 9/6/2024 10:47 PM, Takashi Yano wrote:
>
> defaut should be default in the subject.
>
> > Previously, cygwin read pipe used non-blocking mode althogh non-
> although
>
> > cygwin app uses blocking-mode by default. Despite this requirement,
> > if a cygwin app is executed from a non-cygwwin app and the cygwin
> cygwin
>
> > app exits, read pipe remains on non-blocking mode because of the
> > commit fc691d0246b9. Due to this behaviour, the non-cygwin app
> > cannot read the pipe correctly after that. Similarly, if a non-
> > cygwin app is executed from a cygwin app and the non-cygwin app
> > exits, the read pipe mode remains on blocking mode although cygwin
> > read pipe should be non-blocking mode.
> >
> > These bugs were provoked by pipe mode toggling between cygwin and
> > non-cygwin apps. To make management of pipe mode simpler, this
> > patch has re-designed the pipe implementation. In this new
> > implementation, both read and wrie pipe basically use only blocking
> write
>
> > mode and the behaviour corresponding to the pipe mode is simulated
> > in raw_read() and raw_write(). Only when NtQueryInformationFile(
> put the ( on the next line
>
> > FilePipeLocalInformation) fails for some reasons, the raw_write()
> > cannot simulate non-blocking access. Therefore, the pipe mode is
> > temporarily changed to non-blocking mode.
> >
> > Moreover, because the fact that NtSetInformationFile() in
> > set_pipe_non_blocking(true) fails with STATUS_PIPE_BUSY if the pipe
> > is not empty has been founhd, query handle is not necessary anymore.
> found
Fixed above typos.
> > This allows the implementation much simpler than before.
>
> Yes. Great work!
Thanks!
> > Addresses: https://github.com/git-for-windows/git/issues/5115
> > Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
> > Reported-by: isaacag, Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > ---
> > winsup/cygwin/dtable.cc | 5 +-
> > winsup/cygwin/fhandler/pipe.cc | 482 ++++--------------------
> > winsup/cygwin/local_includes/fhandler.h | 42 +--
> > winsup/cygwin/local_includes/sigproc.h | 1 -
> > winsup/cygwin/select.cc | 25 +-
> > winsup/cygwin/sigproc.cc | 10 -
> > winsup/cygwin/spawn.cc | 4 -
> > 7 files changed, 95 insertions(+), 474 deletions(-)
>
> [...]
>
> > diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> > index c686df650..f99cbbc56 100644
> > --- a/winsup/cygwin/fhandler/pipe.cc
> > +++ b/winsup/cygwin/fhandler/pipe.cc
>
> [...]
>
> > @@ -339,37 +306,11 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
> > FilePipeLocalInformation);
> > if (NT_SUCCESS (status))
> > {
> > - if (fpli.ReadDataAvailable == 0 && nbytes != 0)
> > - break;
> > - }
> > - else if (nbytes != 0)
> > - break;
> > - status = NtReadFile (get_handle (), NULL, NULL, NULL, &io, ptr,
> > - len1, NULL, NULL);
> > - if (isclosed ()) /* A signal handler might have closed the fd. */
> > - {
> > - set_errno (EBADF);
> > - nbytes = (size_t) -1;
> > - }
> > - else if (NT_SUCCESS (status) || status == STATUS_BUFFER_OVERFLOW)
> > - {
> > - nbytes_now = io.Information;
> > - ptr = ((char *) ptr) + nbytes_now;
> > - nbytes += nbytes_now;
> > - if (select_sem && nbytes_now > 0)
> > - release_select_sem ("raw_read");
> > - }
> > - else
> > - {
> > - /* Some errors are not really errors. Detect such cases here. */
> > - switch (status)
> > + if (fpli.ReadDataAvailable == 0)
> > {
> > - case STATUS_END_OF_FILE:
> > - case STATUS_PIPE_BROKEN:
> > - /* This is really EOF. */
> > - break;
> > - case STATUS_PIPE_LISTENING:
> > - case STATUS_PIPE_EMPTY:
> > + if (fpli.NamedPipeState == FILE_PIPE_CLOSING_STATE)
> > + /* Broken pipe ? */
>
> Doesn't "broken pipe" only make sense for writers? For a reader,
> wouldn't this be EOF?
The comment fixed.
> > + break;
> > if (nbytes != 0)
> > break;
> > if (is_nonblocking ())
> > @@ -399,6 +340,34 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
> > break;
> > }
> > continue;
> > + }
> > + }
> > + else if (nbytes != 0)
> > + break;
>
> What if the call to NtQueryInformationFile failed and nbytes == 0? In
> the non-blocking case, I think you need to temporarily set the pipe to
> be non-blocking before calling NtReadFile.
You are right. Added fallback.
> > + status = NtReadFile (get_handle (), NULL, NULL, NULL, &io, ptr,
> > + len1, NULL, NULL);
> > + if (isclosed ()) /* A signal handler might have closed the fd. */
> > + {
> > + set_errno (EBADF);
> > + nbytes = (size_t) -1;
> > + }
> > + else if (NT_SUCCESS (status) || status == STATUS_BUFFER_OVERFLOW)
> > + {
> > + nbytes_now = io.Information;
> > + ptr = ((char *) ptr) + nbytes_now;
> > + nbytes += nbytes_now;
> > + if (select_sem && nbytes_now > 0)
> > + release_select_sem ("raw_read");
> > + }
> > + else
> > + {
> > + /* Some errors are not really errors. Detect such cases here. */
> > + switch (status)
> > + {
> > + case STATUS_END_OF_FILE:
> > + case STATUS_PIPE_BROKEN:
> > + /* This is really EOF. */
> > + break;
> > default:
> > __seterrno_from_nt_status (status);
> > nbytes = (size_t) -1 > @@ -414,18 +383,6 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
> > len = nbytes;
> > }
> >
> > -bool
> > -fhandler_pipe::reader_closed ()
> > -{
> > - if (!query_hdl)
> > - return false;
> > - WaitForSingleObject (hdl_cnt_mtx, INFINITE);
> > - int n_reader = get_obj_handle_count (query_hdl);
> > - int n_writer = get_obj_handle_count (get_handle ());
> > - ReleaseMutex (hdl_cnt_mtx);
> > - return n_reader == n_writer;
> > -}
> > -
>
> Some of the changes below only make sense for pipes, not fifos. Maybe
> we need separate fhandler_pipe::raw_write and fhandle_fifo::raw_write?
Indeed. Let me consider.
> > ssize_t
> > fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
> > {
> > @@ -439,19 +396,45 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
> > if (!len)
> > return 0;
> >
> > - if (reader_closed ())
> > + ssize_t avail = pipe_buf_size;
> > + bool real_non_blocking_mode = false;
> > + if (is_nonblocking ())
> > {
> > - set_errno (EPIPE);
> > - raise (SIGPIPE);
> > - return -1;
> > + FILE_PIPE_LOCAL_INFORMATION fpli;
> > + status = NtQueryInformationFile (get_handle (), &io, &fpli, sizeof fpli,
> > + FilePipeLocalInformation);
> > + if (NT_SUCCESS (status))
> > + {
> > + if (fpli.WriteQuotaAvailable != 0)
> > + avail = fpli.WriteQuotaAvailable;
> > + else /* WriteQuotaAvailable == 0 */
> > + { /* Refer to the comment in select.cc: pipe_data_available(). */
> > + /* NtSetInformationFile() in set_pipe_non_blocking(true) seems
> > + to fail with STATUS_PIPE_BUSY if the pipe is not empty.
> > + In this case, the pipe is really full if WriteQuotaAvailable
> > + is zero. Otherwise, the pipe is empty. */
> > + if (!((fhandler_pipe *)this)->set_pipe_non_blocking (true))
> > + {
> > + /* Full */
> > + set_errno (EAGAIN);
> > + return -1;
> > + }
> > + /* Restore the pipe mode to blocking. */
> > + ((fhandler_pipe *)this)->set_pipe_non_blocking (false);
> > + /* Pipe should be empty because reader is waiting the data. */
> > + }
> > + }
> > + else if (((fhandler_pipe *)this)->set_pipe_non_blocking (true))
> > + /* The pipe space is unknown. */
> > + real_non_blocking_mode = true;
>
> What if set_pipe_non_blocking (true) fails. Do we really want to
> continue, in which case we'll do a blocking write below?
If we want to return an error for this case, what errno is appropriate,
do you think? EIO?
> > }
> >
> > - if (len <= pipe_buf_size || pipe_buf_size == 0)
> > + if (len <= (size_t) avail || pipe_buf_size == 0)
> > chunk = len;
> > else if (is_nonblocking ())
> > - chunk = len = pipe_buf_size;
> > + chunk = len = avail;
> > else
> > - chunk = pipe_buf_size;
> > + chunk = avail;
> >
> > if (!(evt = CreateEvent (NULL, false, false, NULL)))
> > {
>
> [...]
>
> > diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
> > index bc02c3f9d..9d47ff3b0 100644
> > --- a/winsup/cygwin/select.cc
> > +++ b/winsup/cygwin/select.cc
> > @@ -642,7 +642,7 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, int flags)
> > Consequentially, the only reliable information is available on the
> > read side, so fetch info from the read side via the pipe-specific
> > query handle. Use fpli.WriteQuotaAvailable as storage for the actual
> > - interesting value, which is the InboundQuote on the write side,
> > + interesting value, which is the InboundQuota on the write side,
> > decremented by the number of bytes of data in that buffer. */
> > /* Note: Do not use NtQueryInformationFile() for query_hdl because
> > NtQueryInformationFile() seems to interfere with reading pipes
>
> The whole comment needs to be rewritten to reflect the fact that there's
> no longer a query handle.
I will.
--
Takashi Yano <takashi.yano@nifty.ne.jp>
More information about the Cygwin-patches
mailing list