[PATCH v4.2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut

[Ken Brown]

On 9/17/2024 2:22 PM, Ken Brown wrote:
> On 9/17/2024 9:49 AM, Takashi Yano wrote:
>>>> @@ -439,19 +396,45 @@ fhandler_pipe_fifo::raw_write (const void 
>>>> *ptr, size_t len)
>>>>      if (!len)
>>>>        return 0;
>>>> -  if (reader_closed ())
>>>> +  ssize_t avail = pipe_buf_size;
>>>> +  bool real_non_blocking_mode = false;
>>>> +  if (is_nonblocking ())
>>>>        {
>>>> -      set_errno (EPIPE);
>>>> -      raise (SIGPIPE);
>>>> -      return -1;
>>>> +      FILE_PIPE_LOCAL_INFORMATION fpli;
>>>> +      status = NtQueryInformationFile (get_handle (), &io, &fpli, 
>>>> sizeof fpli,
>>>> +                       FilePipeLocalInformation);
>>>> +      if (NT_SUCCESS (status))
>>>> +    {
>>>> +      if (fpli.WriteQuotaAvailable != 0)
>>>> +        avail = fpli.WriteQuotaAvailable;
>>>> +      else /* WriteQuotaAvailable == 0 */
>>>> +        { /* Refer to the comment in select.cc: 
>>>> pipe_data_available(). */
>>>> +          /* NtSetInformationFile() in set_pipe_non_blocking(true) 
>>>> seems
>>>> +         to fail with STATUS_PIPE_BUSY if the pipe is not empty.
>>>> +         In this case, the pipe is really full if WriteQuotaAvailable
>>>> +         is zero. Otherwise, the pipe is empty. */
>>>> +          if (!((fhandler_pipe *)this)->set_pipe_non_blocking (true))
>>>> +        {
>>>> +          /* Full */
>>>> +          set_errno (EAGAIN);
>>>> +          return -1;
>>>> +        }
>>>> +          /* Restore the pipe mode to blocking. */
>>>> +          ((fhandler_pipe *)this)->set_pipe_non_blocking (false);
>>>> +          /* Pipe should be empty because reader is waiting the 
>>>> data. */

One other thing that I missed in my first review.  Is there a possible 
race condition here?  What if the pipe is empty now but another writer 
fills the pipe before we try to write?  Can that happen?  If so, maybe 
it's safer to leave the pipe non-blocking instead of restoring it to 
blocking.

>>>> +        }
>>>> +    }
>>>> +      else if (((fhandler_pipe *)this)->set_pipe_non_blocking (true))
>>>> +    /* The pipe space is unknown. */
>>>> +    real_non_blocking_mode = true;
>>> What if set_pipe_non_blocking (true) fails.  Do we really want to
>>> continue, in which case we'll do a blocking write below?
>> If we want to return an error for this case, what errno is appropriate,
>> do you think? EIO?
> 
> This is a little tricky because there have actually been two errors if 
> we get to this point.  I think EIO is fine except in one case: If one or 
> both errors are due to a broken pipe, we need to use EPIPE (and raise 
> SIGPIPE).  This is easy to test if the first call fails. To test the 
> second, I guess you'd have to modify set_pipe_non_blocking so that it 
> checks for a broken pipe if it fails.  Or maybe have it return a status 
> code that callers can test if they want?  I'm not sure which approach is 
> better.
> 
> Ken