Re: [PATCH] mingw: handle writes to non-blocking pipe
From: Johannes Schindelin <hidden>
Date: 2022-08-10 09:07:45
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.
quoted hunk ↗ jump to hunk
Helped-by: Jeff King [off-list ref] Signed-off-by: René Scharfe <redacted> --- 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.
quoted hunk ↗ jump to hunk
+ #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`.
+ /* 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
quoted hunk ↗ jump to hunk
+ 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