Hi Corinna,

Thanks for reviewing the patches.

On Thu, 16 Sep 2021 15:25:46 +0200
Corinna Vinschen wrote:
> > diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> > index fd2ab87af..7eedc010c 100644
> > --- a/winsup/cygwin/fhandler_pipe.cc
> > +++ b/winsup/cygwin/fhandler_pipe.cc
> > @@ -200,7 +200,14 @@ fhandler_pipe::open_setup (int flags)
> >        SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
> >        read_mtx = CreateMutex (sa, FALSE, NULL);
> >        if (!read_mtx)
> > -	debug_printf ("CreateMutex failed: %E");
> > +	debug_printf ("CreateMutex read_mtx failed: %E");
> > +    }
> > +  if (!hdl_cnt_mtx)
> > +    {
> > +      SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
> > +      hdl_cnt_mtx = CreateMutex (sa, FALSE, NULL);
> > +      if (!hdl_cnt_mtx)
> > +	debug_printf ("CreateMutex hdl_cnt_mtx failed: %E");
> >      }
> >    if (get_dev () == FH_PIPEW && !query_hdl)
> >      set_pipe_non_blocking (is_nonblocking ());
> > @@ -390,8 +397,10 @@ fhandler_pipe_fifo::reader_closed ()
> >  {
> >    if (!query_hdl)
> >      return false;
> > +  cygwait (hdl_cnt_mtx);
> 
> This is missing an `if (hdl_cnt_mtx)' check.

Done.

> Also, given you're using hdl_cnt_mtx only for short-lived blocking,
> you may want to reduce the overhead and just call WFSO with INFINITE
> timeout.
> 
> >    int n_reader = get_obj_handle_count (query_hdl);
> >    int n_writer = get_obj_handle_count (get_handle ());
> > +  ReleaseMutex (hdl_cnt_mtx);
> >    return n_reader == n_writer;
> >  }
> >  
> >  int
> > @@ -576,6 +594,7 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
> >    ftp->set_popen_pid (0);
> >  
> >    int res = 0;
> > +  cygwait (hdl_cnt_mtx);
> 
> Same here and all the other cygwait(hdl_cnt_mtx) calls.

Done.

> It would be great if open_setup() would be converted to a method which
> is allowed to fail, rather than ignoring errors in sync object creation
> and having to test the handle throughout the code.  Given there's only a
> single caller of that function (dtable::init_std_file_from_handle), that
> shouldn't be much work.  But it's certainly better than ignoring creation
> failures in the long run.

I do not have confidence to do this correctly in a short time.
Leave it as a homework. Or may I leave it to you?

> > +      fhs[0]->hdl_cnt_mtx = CreateMutexW (sa, FALSE, NULL);
> > +      if (!fhs[0]->hdl_cnt_mtx)
> > +	{
> > +	  CloseHandle (fhs[0]->read_mtx);
> > +	  CloseHandle (fhs[0]->select_sem);
> > +	  delete fhs[0];
> > +	  NtClose (r);
> > +	  CloseHandle (fhs[1]->select_sem);
> > +	  CloseHandle (fhs[1]->query_hdl);
> > +	  delete fhs[1];
> > +	  NtClose (w);
> > +	  goto out;
> > +	}
> > +      if (!DuplicateHandle (GetCurrentProcess (), fhs[0]->hdl_cnt_mtx,
> > +			    GetCurrentProcess (), &fhs[1]->hdl_cnt_mtx,
> > +			    0, sa->bInheritHandle, DUPLICATE_SAME_ACCESS))
> > +	{
> > +	  CloseHandle (fhs[0]->read_mtx);
> > +	  CloseHandle (fhs[0]->select_sem);
> > +	  CloseHandle (fhs[0]->hdl_cnt_mtx);
> > +	  delete fhs[0];
> > +	  NtClose (r);
> > +	  CloseHandle (fhs[1]->select_sem);
> > +	  CloseHandle (fhs[1]->query_hdl);
> > +	  delete fhs[1];
> > +	  NtClose (w);
> > +	  goto out;
> > +	}
> > +
> >        res = 0;
> >      }
> 
> What about converting this to a goto error chain as in
> fhandler_pty_slave::setup_pseudoconsole?  This makes error handling
> much cleaner, IMHO.

Done.

I have attached the patches revised.

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