Hi Takashi, while I usually would like code comments to be in sync with the code (and therefore have it be changed in the same commit), in this instance I totally agree that it made sense to refactor them out into their own patch because they are quite extensive and would otherwise have drowned out the functional changes.
Thank you, Johannes On Fri, 27 Jun 2025, Takashi Yano wrote: > The commit "Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader" > modifies how the amount of writable data is evaluated. This patch > updates the source comments to align with that change. > > Reviewed-by: > Signed-off-by: Takashi Yano <[email protected]> > --- > winsup/cygwin/fhandler/pipe.cc | 8 ++--- > winsup/cygwin/select.cc | 54 ++++++++++++++++++---------------- > 2 files changed, 31 insertions(+), 31 deletions(-) > > diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc > index bda0a9b25..22addbb18 100644 > --- a/winsup/cygwin/fhandler/pipe.cc > +++ b/winsup/cygwin/fhandler/pipe.cc > @@ -497,8 +497,8 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t > len) > { /* 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. */ > + In this case, WriteQuotaAvailable indicates real pipe space. > + Otherwise, the pipe is empty. */ > status = fh->set_pipe_non_blocking (true); > if (NT_SUCCESS (status)) > /* Pipe should be empty because reader is waiting for data. */ > @@ -655,9 +655,7 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t > len) > if (io.Information > 0 || len <= PIPE_BUF || short_write_once) > break; > /* Independent of being blocking or non-blocking, if we're here, > - the pipe has less space than requested. If the pipe is a > - non-Cygwin pipe, just try the old strategy of trying a half > - write. If the pipe has at > + the pipe has less space than requested. If the pipe has at > least PIPE_BUF bytes available, try to write all matching > PIPE_BUF sized blocks. If it's less than PIPE_BUF, try > the next less power of 2 bytes. This is not really the Linux > diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc > index 0b9afb359..701f4d9a6 100644 > --- a/winsup/cygwin/select.cc > +++ b/winsup/cygwin/select.cc > @@ -623,32 +623,34 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE > h, int mode) > if (mode == PDA_WRITE) > { > /* If there is anything available in the pipe buffer then signal > - that. This means that a pipe could still block since you could > - be trying to write more to the pipe than is available in the > - buffer but that is the hazard of select(). > - > - Note that WriteQuotaAvailable is unreliable. > - > - Usually WriteQuotaAvailable on the write side reflects the space > - available in the inbound buffer on the read side. However, if a > - pipe read is currently pending, WriteQuotaAvailable on the write side > - is decremented by the number of bytes the read side is requesting. > - So it's possible (even likely) that WriteQuotaAvailable is 0, even > - if the inbound buffer on the read side is not full. This can lead to > - a deadlock situation: The reader is waiting for data, but select > - on the writer side assumes that no space is available in the read > - side inbound buffer. > - > - Consequentially, there are two possibilities when WriteQuotaAvailable > - is 0. One is that the buffer is really full. The other is that the > - reader is currently trying to read the pipe and it is pending. > - In the latter case, the fact that the reader cannot read the data > - immediately means that the pipe is empty. In the former case, > - NtSetInformationFile() in set_pipe_non_blocking(true) will fail > - with STATUS_PIPE_BUSY, while it succeeds in the latter case. > - Therefore, we can distinguish these cases by calling set_pipe_non_ > - blocking(true). If it returns success, the pipe is empty, so we > - return the pipe buffer size. Otherwise, we return 0. */ > + that. This means that a pipe could still block since you could > + be trying to write more to the pipe than is available in the > + buffer but that is the hazard of select(). > + > + Note that WriteQuotaAvailable is unreliable. > + > + Usually WriteQuotaAvailable on the write side reflects the space > + available in the inbound buffer on the read side. However, if a > + pipe read is currently pending, WriteQuotaAvailable on the write side > + is decremented by the number of bytes the read side is requesting. > + So it's possible (even likely) that WriteQuotaAvailable is less than > + actual space available in the pipe, even if the inbound buffer is > + empty. This can lead to a deadlock situation: The reader is waiting > + for data, but select on the writer side assumes that no space is > + available in the read side inbound buffer. > + > + Consequentially, there are two possibilities when WriteQuotaAvailable > + is less than pipe size. One is that the buffer is really not empty. > + The other is that the reader is currently trying to read the pipe > + and it is pending. > + In the latter case, the fact that the reader cannot read the data > + immediately means that the pipe is empty. In the former case, > + NtSetInformationFile() in set_pipe_non_blocking(true) will fail > + with STATUS_PIPE_BUSY, while it succeeds in the latter case. > + Therefore, we can distinguish these cases by calling set_pipe_non_ > + blocking(true). If it returns success, the pipe is empty, so we > + return the pipe buffer size. Otherwise, we return the value of > + WriteQuotaAvailable as is. */ > if (fh->get_device () == FH_PIPEW > && fpli.WriteQuotaAvailable < fpli.InboundQuota) > { > -- > 2.45.1 > >
