[PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.
Corinna Vinschen
corinna-cygwin@cygwin.com
Mon Mar 11 20:33:04 GMT 2024
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
More information about the Cygwin-patches
mailing list