[PATCH v2] Cygwin: pipe: Simplify raw_write() a bit (stop to use chunk)

[Johannes Schindelin]

Hi Takashi,

On Wed, 25 Jun 2025, Takashi Yano wrote:

> Reviewed-by:
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>

This is way too terse. There is a difference between being succinct and
leaving things unsaid.

Also, please make sure that v2 is a reply to v1 of the patch. I almost
commented on v1 by mistake.

> ---
>  winsup/cygwin/fhandler/pipe.cc | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index e35d523bb..c35411abf 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -443,7 +443,6 @@ ssize_t
>  fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
>  {
>    size_t nbytes = 0;
> -  ULONG chunk;

Okay, removing this local variable is a good indicator that this diff
shows all the related logic, without having to resort to looking at the
entire `pipe.cc` file that is not reproduced in this email.

>    NTSTATUS status = STATUS_SUCCESS;
>    IO_STATUS_BLOCK io;
>    HANDLE evt;
> @@ -540,11 +539,6 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
>  	}
>      }
>  
> -  if (len <= (size_t) avail)
> -    chunk = len;
> -  else
> -    chunk = avail;
> -
>    if (!(evt = CreateEvent (NULL, false, false, NULL)))
>      {
>        __seterrno ();
> @@ -561,8 +555,8 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
>        ULONG len1;
>        DWORD waitret = WAIT_OBJECT_0;
>  
> -      if (left > chunk && !is_nonblocking ())
> -	len1 = chunk;
> +      if (left > (size_t) avail && !is_nonblocking ())
> +	len1 = (ULONG) avail;
>        else
>  	len1 = (ULONG) left;

So there is a subtle change here, which _should_ result in the same
behavior, but it is far from obvious.

If both `left` and `len` are larger than `avail`, the behavior is
obviously the same as before because `len1` is clamped to `avail` in that
instance.

If `left` is smaller than `len`, and `len` is smaller than `avail`, `len1`
is clamped to `left`, same as before. But even if `left < avail < len`,
`chunk` would have previously been set to `avail` and the behavior is the
same.

Since the variable `left` is defined as `left = len - nbytes`, it is never
larger than `len`.

Without giving a simple overview of this in the commit message, every
reader will have to (re-)reason out this finding, which is suboptimal.

Please improve the commit message, for that reason.

Also note that this patch conflicts very much with
https://inbox.sourceware.org/cygwin-patches/62e79c50daf4e3ae28db3ae1a3cf52460f0d8968.1750775114.git.johannes.schindelin@gmx.de/
and it would therefore make most sense to focus first on landing that
patch, then redo v2 of this here patch on top.

Ciao,
Johannes