Re: [PATCH] mingw: handle writes to non-blocking pipe

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 10, 2022, 09:07 UTC
Message-ID: <1739sr2q-71p7-q601-2022-ps56r12pos50@tzk.qr>
In-Reply-To: <72d007c5-9429-1612-24d7-af5db906dd63@web.de>

Hi René,

On Wed, 10 Aug 2022, René Scharfe wrote:

> write() on Windows reports ENOSPC when writing to a non-blocking pipe
> whose buffer is full and rejects writes bigger than the buffer outright.
> Change the error code to EAGAIN and try a buffer-sized partial write to
> comply with POSIX and the expections of our Git-internal callers.

Excellent analysis, thank you!

However, let's reword this to clarify that the error code is set to EAGAIN only if the buffer-sized partial write fails.

Show 16 quoted lines

>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  compat/mingw.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index b5502997e2..c6f244c0fe 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -689,6 +689,8 @@ int mingw_fflush(FILE *stream)
>  	return ret;
>  }
>
> +#define PIPE_BUFFER_SIZE (8192)

This constant hails all the way back from 897bb8cb2c2 (Windows: A pipe() replacement whose ends are not inherited to children., 2007-12-07), in case anyone wondered like I did where that number came from (and why it is so low).

It is outside the purview of this patch to change that constant, therefore I am fine with leaving this as-is.

Show 9 quoted lines

> +
>  #undef write
>  ssize_t mingw_write(int fd, const void *buf, size_t len)
>  {
> @@ -702,6 +704,21 @@ ssize_t mingw_write(int fd, const void *buf, size_t len)
>  		else
>  			errno = EINVAL;
>  	}
> +	if (result < 0 && errno == ENOSPC) {

It might make the code clearer to turn this into an `else if`.

Show 11 quoted lines

> +		/* check if fd is a non-blocking pipe */
> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> +		DWORD s;
> +		if (GetFileType(h) == FILE_TYPE_PIPE &&
> +		    GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&
> +		    (s & PIPE_NOWAIT)) {
> +			DWORD obuflen;
> +			if (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))
> +				obuflen = PIPE_BUFFER_SIZE;
> +			if (len > obuflen)
> +				return mingw_write(fd, buf, obuflen);

It is probably easier to reason about to recurse instead of using a `goto` here.

Thank you for this patch! Dscho

Show 18 quoted lines

> +			errno = EAGAIN;
> +		}
> +	}
>
>  	return result;
>  }
> @@ -1079,7 +1096,7 @@ int pipe(int filedes[2])
>  	HANDLE h[2];
>
>  	/* this creates non-inheritable handles */
> -	if (!CreatePipe(&h[0], &h[1], NULL, 8192)) {
> +	if (!CreatePipe(&h[0], &h[1], NULL, PIPE_BUFFER_SIZE)) {
>  		errno = err_win_to_posix(GetLastError());
>  		return -1;
>  	}
> --
> 2.37.1.windows.1
>