Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 8, 2022, 12:55 UTC
Message-ID: <099911o8-43q1-o29s-5s36-34r5qn37nq8n@tzk.qr>
In-Reply-To: <362560ca-1e37-bab3-e508-4d5bf5429e66@web.de>

Hi René,

On Sun, 7 Aug 2022, René Scharfe wrote:

Show 44 quoted lines

> Am 05.08.2022 um 17:36 schrieb Jeff King:
> > On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:
> > >>  int enable_nonblock(int fd)
> >>  {
> >> +	DWORD mode;
> >> +	HANDLE handle = winansi_get_osfhandle(fd);
> >> +	if (!handle)
> >> +		return -1;
> >> +	if (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))
> >> +		return -1;
> >> +	if (mode & PIPE_NOWAIT)
> >> +		return 0;
> >> +	mode |= PIPE_NOWAIT;
> >> +	if (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))
> >> +		return -1;
> >>  	return 0;
> >>  }
> >
> > This looks plausibly correct to me. ;) We might want to change the name
> > of the compat layer to enable_pipe_nonblock(), since one assumes from
> > the function names this only works for pipes.
>
> Or how about this?  Squashable.  Needs testing.
>
> --- >8 ---
> Subject: [PATCH] nonblock: support Windows
>
> Implement enable_nonblock() using the Windows API.  Supports only named
> and anonymous pipes for now, which suffices for the current caller.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  compat/nonblock.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/compat/nonblock.c b/compat/nonblock.c
> index 897c099010..78923cd2c3 100644
> --- a/compat/nonblock.c
> +++ b/compat/nonblock.c
> @@ -14,9 +14,40 @@ int enable_nonblock(int fd)
>
>  #else
>
> +#ifdef GIT_WINDOWS_NATIVE

Maybe use an `#elif defined(GIT_WINDOWS_NATIVE)` here? That would make the code structures clearer, methinks.

Show 20 quoted lines

> +
> +#include "win32.h"
> +
> +int enable_nonblock(int fd)
> +{
> +	HANDLE h = (HANDLE)_get_osfhandle(fd);
> +	DWORD mode;
> +	DWORD type = GetFileType(h);
> +	if (type == FILE_TYPE_UNKNOWN && GetLastError() != NO_ERROR) {
> +		errno = EBADF;
> +		return -1;
> +	}
> +	if (type != FILE_TYPE_PIPE)
> +		BUG("unsupported file type: %lu", type);
> +	if (!GetNamedPipeHandleState(h, &mode, NULL, NULL, NULL, NULL, 0)) {
> +		errno = err_win_to_posix(GetLastError());
> +		return -1;
> +	}
> +	mode |= PIPE_NOWAIT;
> +	if (!SetNamedPipeHandleState(h, &mode, NULL, NULL)) {

Nice.

FWIW the documentation of `PIPE_NOWAIT` says:

	Note that nonblocking mode is supported for compatibility with
	Microsoft LAN Manager version 2.0 and should not be used to
	achieve asynchronous input and output (I/O) with named pipes.

(see https://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate for full details)

There is little more information to be found on the interwebs, the closest to an in-depth explanation is here: https://devblogs.microsoft.com/oldnewthing/20110114-00/?p=11753

Even if that comment suggests that this mode is deprecated, I think it is safe to rely on it in Git's source code.

Thanks, Dscho

Show 19 quoted lines

> +		errno = err_win_to_posix(GetLastError());
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +#else
> +
>  int enable_nonblock(int fd)
>  {
>  	return 0;
>  }
>
>  #endif
> +
> +#endif
> --
> 2.37.1.windows.1
>