On Mon, 11 Mar 2024 21:33:04 +0100
Corinna Vinschen wrote:
> 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 <wh9692@protonmail.com>
> > Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
> > Signed-off-by: Takashi Yano <takashi.yano@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().

Fixed.

> > +  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?

Yeah, you are right. It is not too late to set non-blocking mode
in fixup_after_exec(). Thanks.

> > +
> > +	/* 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
>                                      ^^^^^^^

Fixed.

Please check v3 patch attached.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>