Hi Takashi,
this looks much better. Just one question and a few comment changes... On Mar 11 22:18, Takashi Yano wrote: > Subject: [PATCH v2] Cygwin: pipe: Make sure to set read pipe non-blocking for > cygwin apps. > > If pipe reader is a non-cygwin app first, and cygwin process reads > the same pipe after that, the pipe has been set to bclocking mode > for the cygwin app. However, the commit 9e4d308cd592 assumes the > pipe for cygwin process always is non-blocking mode. With this patch, > the pipe mode is reset to non-blocking when cygwin app is started. > > Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html > Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag > for read pipe.") > Reported-by: wh <wh9...@protonmail.com> > Reviewed-by: Corinna Vinschen <cori...@vinschen.de> > Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp> > --- > winsup/cygwin/fhandler/pipe.cc | 54 +++++++++++++++++++++++++ > winsup/cygwin/local_includes/fhandler.h | 2 + > winsup/cygwin/spawn.cc | 35 +--------------- > 3 files changed, 58 insertions(+), 33 deletions(-) > > diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc > index 29d3b41d9..b29726dcb 100644 > --- a/winsup/cygwin/fhandler/pipe.cc > +++ b/winsup/cygwin/fhandler/pipe.cc > @@ -18,6 +18,7 @@ details. */ > #include "pinfo.h" > #include "shared_info.h" > #include "tls_pbuf.h" > +#include "sigproc.h" > #include <assert.h> > > /* This is only to be used for writing. When reading, > @@ -1288,3 +1289,56 @@ close_proc: > } > return NULL; > } > + > +void > +fhandler_pipe::spawn_worker (bool iscygwin, int fileno_stdin, > + int fileno_stdout, int fileno_stderr) > +{ > + bool need_send_noncygchld_sig = false; > + > + /* Set read pipe itself always non-blocking for cygwin process. blocking/ > + non-blocking is simulated in the raw_read(). As for write pipe, follow > + the is_nonblocking(). */ You can drop the articles here, i.e. ...non-blocking is simulated in raw_read(). For write pipe follow is_nonblocking(). > + int fd; > + cygheap_fdenum cfd (false); > + while ((fd = cfd.next ()) >= 0) > + if (cfd->get_dev () == FH_PIPEW > + && (fd == fileno_stdout || fd == fileno_stderr)) > + { > + fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd; > + bool mode = iscygwin ? pipe->is_nonblocking () : false; > + pipe->set_pipe_non_blocking (mode); What bugs me here is that we now run the loop for cygwin children as well. The old code only did that for non-cygwin children. This looks like quite a performance hit, potentially. Especially if the process has many open descriptors. Let's say, a recursive make or so. Did you find this is necessary? No way to avoid that? > + > + /* Setup for query_ndl stuff. Read the comment below. */ > + if (!iscygwin && pipe->request_close_query_hdl ()) > + need_send_noncygchld_sig = true; > + } > + else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin) > + { > + fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd; > + pipe->set_pipe_non_blocking (iscygwin); > + } > + > + /* If multiple writers including non-cygwin app exist, the non-cygwin > + app cannot detect pipe closure on the read side when the pipe is > + created by system account or the the pipe creator is running as ^^^^^^^ Thanks, Corinna