Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

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 <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().

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