[PATCH] mingw: emulate write(2) that fails with a EPIPE

[PATCH] mingw: emulate write(2) that fails with a EPIPE

[Johannes Schindelin]

On Windows, when writing to a pipe fails, errno is always
EINVAL. However, Git expects it to be EPIPE.

According to the documentation, there are two cases in which write()
triggers EINVAL: the buffer is NULL, or the length is odd but the mode
is 16-bit Unicode (the broken pipe is not mentioned as possible cause).
Git never sets the file mode to anything but binary, therefore we know
that errno should actually be EPIPE if it is EINVAL and the buffer is
not NULL.

See https://msdn.microsoft.com/en-us/library/1570wh78.aspx for more
details.

This works around t5571.11 failing with v2.6.4 on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/compat/mingw.h b/compat/mingw.h
index 738865c..2aca347 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -210,6 +210,24 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);
 int mingw_fflush(FILE *stream);
 #define fflush mingw_fflush
 
+static inline ssize_t mingw_write(int fd, const void *buf, size_t len)
+{
+	ssize_t result = write(fd, buf, len);
+
+	if (result < 0 && errno == EINVAL && buf) {
+		/* check if fd is a pipe */
+		HANDLE h = (HANDLE) _get_osfhandle(fd);
+		if (GetFileType(h) == FILE_TYPE_PIPE)
+			errno = EPIPE;
+		else
+			errno = EINVAL;
+	}
+
+	return result;
+}
+
+#define write mingw_write
+
 int mingw_access(const char *filename, int mode);
 #undef access
 #define access mingw_access
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH] mingw: emulate write(2) that fails with a EPIPE

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> On Windows, when writing to a pipe fails, errno is always
> EINVAL. However, Git expects it to be EPIPE.
>
> According to the documentation, there are two cases in which write()
> triggers EINVAL: the buffer is NULL, or the length is odd but the mode
> is 16-bit Unicode (the broken pipe is not mentioned as possible cause).
> Git never sets the file mode to anything but binary, therefore we know
> that errno should actually be EPIPE if it is EINVAL and the buffer is
> not NULL.

Makes sense.

>  int mingw_fflush(FILE *stream);
>  #define fflush mingw_fflush
>  
> +static inline ssize_t mingw_write(int fd, const void *buf, size_t len)
> +{
> +	ssize_t result = write(fd, buf, len);
> +
> +	if (result < 0 && errno == EINVAL && buf) {
> +		/* check if fd is a pipe */
> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> +		if (GetFileType(h) == FILE_TYPE_PIPE)
> +			errno = EPIPE;
> +		else
> +			errno = EINVAL;
> +	}
> +
> +	return result;
> +}
> +
> +#define write mingw_write
> +

It strikes me a bit strange to see this inlined compared to what
appears in the context.  Shouldn't the implementation be done in
compat/mingw.c like all others?

>  int mingw_access(const char *filename, int mode);
>  #undef access
>  #define access mingw_access

Re: [PATCH] mingw: emulate write(2) that fails with a EPIPE

[Johannes Schindelin]

Hi Junio,

On Wed, 16 Dec 2015, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> >  int mingw_fflush(FILE *stream);
> >  #define fflush mingw_fflush
> >  
> > +static inline ssize_t mingw_write(int fd, const void *buf, size_t len)
> > +{
> > +	ssize_t result = write(fd, buf, len);
> > +
> > +	if (result < 0 && errno == EINVAL && buf) {
> > +		/* check if fd is a pipe */
> > +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> > +		if (GetFileType(h) == FILE_TYPE_PIPE)
> > +			errno = EPIPE;
> > +		else
> > +			errno = EINVAL;
> > +	}
> > +
> > +	return result;
> > +}
> > +
> > +#define write mingw_write
> > +
> 
> It strikes me a bit strange to see this inlined compared to what
> appears in the context.  Shouldn't the implementation be done in
> compat/mingw.c like all others?

My intuition (which I honestly did not verify using performance tests) was
that write() is called *much* more often than, say, open(), and therefore
I wanted to interfere as little as possible with the performance penalty.
Hence the choice of an inlined function as opposed to a non-optimizable
increment of the call chain.

If it bothers you a lot I will set aside time to perform performance
tests.

Ciao,
Dscho

Re: [PATCH] mingw: emulate write(2) that fails with a EPIPE

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> My intuition (which I honestly did not verify using performance tests) was
> that write() is called *much* more often than, say, open(),...

My gut feeling agrees with yours, but I do not think the frequency
at which write() is called should be the primary factor when you
decide to make its wrapper inlined.  Once you call write(2), you
will hit either the disk or the network doing I/O, and at that point
I'd expect that the cost of making an extra layer of wrapper call
would be lost in the noise.  I'd worry a lot more about from how
many callsites write() is called---by inlining the extra code that
is run only when the underlying write(2) returns an error to many
callsites, we would make the program as a whole bigger, and as the
result other code needs to be evicted out of the instruction cache,
which also would hurt performance.

Re: [PATCH] mingw: emulate write(2) that fails with a EPIPE

[Johannes Schindelin]

Hi Junio,

On Thu, 17 Dec 2015, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > My intuition (which I honestly did not verify using performance tests) was
> > that write() is called *much* more often than, say, open(),...
> 
> My gut feeling agrees with yours, but I do not think the frequency
> at which write() is called should be the primary factor when you
> decide to make its wrapper inlined.  Once you call write(2), you
> will hit either the disk or the network doing I/O, and at that point
> I'd expect that the cost of making an extra layer of wrapper call
> would be lost in the noise.  I'd worry a lot more about from how
> many callsites write() is called---by inlining the extra code that
> is run only when the underlying write(2) returns an error to many
> callsites, we would make the program as a whole bigger, and as the
> result other code needs to be evicted out of the instruction cache,
> which also would hurt performance.

That argument convinced me. v2 coming shortly.

Ciao,
Dscho

Re: [PATCH] mingw: emulate write(2) that fails with a EPIPE

[Eric Sunshine]

On Wednesday, December 16, 2015, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> On Windows, when writing to a pipe fails, errno is always
> EINVAL. However, Git expects it to be EPIPE.
>
> According to the documentation, there are two cases in which write()
> triggers EINVAL: the buffer is NULL, or the length is odd but the mode
> is 16-bit Unicode (the broken pipe is not mentioned as possible cause).
> Git never sets the file mode to anything but binary, therefore we know
> that errno should actually be EPIPE if it is EINVAL and the buffer is
> not NULL.
>
> See https://msdn.microsoft.com/en-us/library/1570wh78.aspx for more
> details.
>
> This works around t5571.11 failing with v2.6.4 on Windows.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/compat/mingw.h b/compat/mingw.h
> @@ -210,6 +210,24 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);
> +static inline ssize_t mingw_write(int fd, const void *buf, size_t len)
> +{
> +       ssize_t result = write(fd, buf, len);
> +
> +       if (result < 0 && errno == EINVAL && buf) {

Here, errno is EINVAL...

> +               /* check if fd is a pipe */
> +               HANDLE h = (HANDLE) _get_osfhandle(fd);
> +               if (GetFileType(h) == FILE_TYPE_PIPE)
> +                       errno = EPIPE;
> +               else
> +                       errno = EINVAL;

Does any of the code between the outer 'if' and this point clobber
errno, or are you merely assigning EINVAL for robustness against
future changes?

> +       }
> +
> +       return result;
> +}
> +
> +#define write mingw_write
> +
> --
> 2.6.3.windows.1.300.g1c25e49

Re: [PATCH] mingw: emulate write(2) that fails with a EPIPE

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       if (result < 0 && errno == EINVAL && buf) {
>
> Here, errno is EINVAL...
>
>> +               /* check if fd is a pipe */
>> +               HANDLE h = (HANDLE) _get_osfhandle(fd);
>> +               if (GetFileType(h) == FILE_TYPE_PIPE)
>> +                       errno = EPIPE;
>> +               else
>> +                       errno = EINVAL;
>
> Does any of the code between the outer 'if' and this point clobber
> errno, or are you merely assigning EINVAL for robustness against
> future changes?

I do think it is a good idea to reassign 'errno' here to stress on
the fact that we return EPIPE only when we positively know that we
were reading from a pipe, and otherwise we won't change the original
errno at all.

But at the same time, if other things that are called before we
figure out if we were reading from a pipe could affect errno, I
wonder if that is an indication of a bug--an error return from
system functions that the code is not responding to.  For example,
can _get_osfhandle() fail and when it does what would we see?
Perhaps NULL in h?

Re: [PATCH] mingw: emulate write(2) that fails with a EPIPE

[Johannes Schindelin]

Hi Eric,

On Wed, 16 Dec 2015, Eric Sunshine wrote:

> On Wednesday, December 16, 2015, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > diff --git a/compat/mingw.h b/compat/mingw.h
> > @@ -210,6 +210,24 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);
> > +static inline ssize_t mingw_write(int fd, const void *buf, size_t len)
> > +{
> > +       ssize_t result = write(fd, buf, len);
> > +
> > +       if (result < 0 && errno == EINVAL && buf) {
> 
> Here, errno is EINVAL...
> 
> > +               /* check if fd is a pipe */
> > +               HANDLE h = (HANDLE) _get_osfhandle(fd);
> > +               if (GetFileType(h) == FILE_TYPE_PIPE)
> > +                       errno = EPIPE;
> > +               else
> > +                       errno = EINVAL;
> 
> Does any of the code between the outer 'if' and this point clobber
> errno, or are you merely assigning EINVAL for robustness against
> future changes?

Yes, it is proofing the code against future changes. And TBH I did not
even bother to check whether _get_osfhandle() or GetFileType() can modify
the errno value, since I *really* wanted to make sure that errno is either
EPIPE or EINVAL in this execution path.

Ciao,
Dscho

[PATCH v2] mingw: emulate write(2) that fails with a EPIPE

[Johannes Schindelin]

On Windows, when writing to a pipe fails, errno is always
EINVAL. However, Git expects it to be EPIPE.

According to the documentation, there are two cases in which write()
triggers EINVAL: the buffer is NULL, or the length is odd but the mode
is 16-bit Unicode (the broken pipe is not mentioned as possible cause).
Git never sets the file mode to anything but binary, therefore we know
that errno should actually be EPIPE if it is EINVAL and the buffer is
not NULL.

See https://msdn.microsoft.com/en-us/library/1570wh78.aspx for more
details.

This works around t5571.11 failing with v2.6.4 on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 17 +++++++++++++++++
 compat/mingw.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 90bdb1e..5edea29 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -394,6 +394,23 @@ int mingw_fflush(FILE *stream)
 	return ret;
 }
 
+#undef write
+ssize_t mingw_write(int fd, const void *buf, size_t len)
+{
+	ssize_t result = write(fd, buf, len);
+
+	if (result < 0 && errno == EINVAL && buf) {
+		/* check if fd is a pipe */
+		HANDLE h = (HANDLE) _get_osfhandle(fd);
+		if (GetFileType(h) == FILE_TYPE_PIPE)
+			errno = EPIPE;
+		else
+			errno = EINVAL;
+	}
+
+	return result;
+}
+
 int mingw_access(const char *filename, int mode)
 {
 	wchar_t wfilename[MAX_PATH];
diff --git a/compat/mingw.h b/compat/mingw.h
index 738865c..57ca477 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -210,6 +210,9 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);
 int mingw_fflush(FILE *stream);
 #define fflush mingw_fflush
 
+ssize_t mingw_write(int fd, const void *buf, size_t len);
+#define write mingw_write
+
 int mingw_access(const char *filename, int mode);
 #undef access
 #define access mingw_access
Interdiff vs v1:

diff --git a/compat/mingw.c b/compat/mingw.c
index 90bdb1e..5edea29 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -394,6 +394,23 @@ int mingw_fflush(FILE *stream)
 	return ret;
 }
 
+#undef write
+ssize_t mingw_write(int fd, const void *buf, size_t len)
+{
+	ssize_t result = write(fd, buf, len);
+
+	if (result < 0 && errno == EINVAL && buf) {
+		/* check if fd is a pipe */
+		HANDLE h = (HANDLE) _get_osfhandle(fd);
+		if (GetFileType(h) == FILE_TYPE_PIPE)
+			errno = EPIPE;
+		else
+			errno = EINVAL;
+	}
+
+	return result;
+}
+
 int mingw_access(const char *filename, int mode)
 {
 	wchar_t wfilename[MAX_PATH];
diff --git a/compat/mingw.h b/compat/mingw.h
index 2aca347..57ca477 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -210,22 +210,7 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);
 int mingw_fflush(FILE *stream);
 #define fflush mingw_fflush
 
-static inline ssize_t mingw_write(int fd, const void *buf, size_t len)
-{
-	ssize_t result = write(fd, buf, len);
-
-	if (result < 0 && errno == EINVAL && buf) {
-		/* check if fd is a pipe */
-		HANDLE h = (HANDLE) _get_osfhandle(fd);
-		if (GetFileType(h) == FILE_TYPE_PIPE)
-			errno = EPIPE;
-		else
-			errno = EINVAL;
-	}
-
-	return result;
-}
-
+ssize_t mingw_write(int fd, const void *buf, size_t len);
 #define write mingw_write
 
 int mingw_access(const char *filename, int mode);

-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v2] mingw: emulate write(2) that fails with a EPIPE

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> On Windows, when writing to a pipe fails, errno is always
> EINVAL. However, Git expects it to be EPIPE.
>
> According to the documentation, there are two cases in which write()
> triggers EINVAL: the buffer is NULL, or the length is odd but the mode
> is 16-bit Unicode (the broken pipe is not mentioned as possible cause).
> Git never sets the file mode to anything but binary, therefore we know
> that errno should actually be EPIPE if it is EINVAL and the buffer is
> not NULL.
>
> See https://msdn.microsoft.com/en-us/library/1570wh78.aspx for more
> details.
>
> This works around t5571.11 failing with v2.6.4 on Windows.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/mingw.c | 17 +++++++++++++++++
>  compat/mingw.h |  3 +++
>  2 files changed, 20 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 90bdb1e..5edea29 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -394,6 +394,23 @@ int mingw_fflush(FILE *stream)
>  	return ret;
>  }
>  
> +#undef write
> +ssize_t mingw_write(int fd, const void *buf, size_t len)
> +{
> +	ssize_t result = write(fd, buf, len);
> +
> +	if (result < 0 && errno == EINVAL && buf) {
> +		/* check if fd is a pipe */
> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> +		if (GetFileType(h) == FILE_TYPE_PIPE)
> +			errno = EPIPE;
> +		else
> +			errno = EINVAL;
> +	}
> +
> +	return result;
> +}
> +
>  int mingw_access(const char *filename, int mode)
>  {
>  	wchar_t wfilename[MAX_PATH];
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 738865c..57ca477 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -210,6 +210,9 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);
>  int mingw_fflush(FILE *stream);
>  #define fflush mingw_fflush
>  
> +ssize_t mingw_write(int fd, const void *buf, size_t len);
> +#define write mingw_write
> +
>  int mingw_access(const char *filename, int mode);
>  #undef access
>  #define access mingw_access


PLEASE DON'T DO THE BELOW TO THE SAME MESSAGE AS THE PATCH ITSELF.
"git apply" would not read and understand the next line as a natural
language sentence for obvious reasons.

> Interdiff vs v1:
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 90bdb1e..5edea29 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -394,6 +394,23 @@ int mingw_fflush(FILE *stream)
>  	return ret;
>  }
>  
> +#undef write
> +ssize_t mingw_write(int fd, const void *buf, size_t len)
> +{
> +	ssize_t result = write(fd, buf, len);
> +
> +	if (result < 0 && errno == EINVAL && buf) {
> +		/* check if fd is a pipe */
> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> +		if (GetFileType(h) == FILE_TYPE_PIPE)
> +			errno = EPIPE;
> +		else
> +			errno = EINVAL;
> +	}
> +
> +	return result;
> +}
> +
>  int mingw_access(const char *filename, int mode)
>  {
>  	wchar_t wfilename[MAX_PATH];
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 2aca347..57ca477 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -210,22 +210,7 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);
>  int mingw_fflush(FILE *stream);
>  #define fflush mingw_fflush
>  
> -static inline ssize_t mingw_write(int fd, const void *buf, size_t len)
> -{
> -	ssize_t result = write(fd, buf, len);
> -
> -	if (result < 0 && errno == EINVAL && buf) {
> -		/* check if fd is a pipe */
> -		HANDLE h = (HANDLE) _get_osfhandle(fd);
> -		if (GetFileType(h) == FILE_TYPE_PIPE)
> -			errno = EPIPE;
> -		else
> -			errno = EINVAL;
> -	}
> -
> -	return result;
> -}
> -
> +ssize_t mingw_write(int fd, const void *buf, size_t len);
>  #define write mingw_write
>  
>  int mingw_access(const char *filename, int mode);

Re: [PATCH v2] mingw: emulate write(2) that fails with a EPIPE

[Johannes Schindelin]

Hi Junio,

On Thu, 17 Dec 2015, Junio C Hamano wrote:

> PLEASE DON'T DO THE BELOW TO THE SAME MESSAGE AS THE PATCH ITSELF.
> "git apply" would not read and understand the next line as a natural
> language sentence for obvious reasons.
> 
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Interdiff vs v1:

Whoops. This is an obvious bug in my patch series submission script. Will
fix.

Sorry,
Dscho

Re: [PATCH v2] mingw: emulate write(2) that fails with a EPIPE

[Johannes Sixt]

Am 17.12.2015 um 18:08 schrieb Johannes Schindelin:
> On Windows, when writing to a pipe fails, errno is always
> EINVAL. However, Git expects it to be EPIPE.
>
> According to the documentation, there are two cases in which write()
> triggers EINVAL: the buffer is NULL, or the length is odd but the mode
> is 16-bit Unicode (the broken pipe is not mentioned as possible cause).
> Git never sets the file mode to anything but binary, therefore we know
> that errno should actually be EPIPE if it is EINVAL and the buffer is
> not NULL.
>
> See https://msdn.microsoft.com/en-us/library/1570wh78.aspx for more
> details.
>
> This works around t5571.11 failing with v2.6.4 on Windows.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/mingw.c | 17 +++++++++++++++++
>   compat/mingw.h |  3 +++
>   2 files changed, 20 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 90bdb1e..5edea29 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -394,6 +394,23 @@ int mingw_fflush(FILE *stream)
>   	return ret;
>   }
>
> +#undef write
> +ssize_t mingw_write(int fd, const void *buf, size_t len)
> +{
> +	ssize_t result = write(fd, buf, len);
> +
> +	if (result < 0 && errno == EINVAL && buf) {
> +		/* check if fd is a pipe */
> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> +		if (GetFileType(h) == FILE_TYPE_PIPE)
> +			errno = EPIPE;
> +		else
> +			errno = EINVAL;
> +	}
> +
> +	return result;
> +}
> +
>   int mingw_access(const char *filename, int mode)
>   {
>   	wchar_t wfilename[MAX_PATH];

Looks good. I tested the patch, and it fixes the failure exposed by 
t5571.11.

Acked-by: Johannes Sixt <j6t@kdbg.org>

Thanks!

-- Hannes

Re: [PATCH v2] mingw: emulate write(2) that fails with a EPIPE

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> +#undef write
>> +ssize_t mingw_write(int fd, const void *buf, size_t len)
>> +{
>> +	ssize_t result = write(fd, buf, len);
>> +
>> +	if (result < 0 && errno == EINVAL && buf) {
>> +		/* check if fd is a pipe */
>> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
>> +		if (GetFileType(h) == FILE_TYPE_PIPE)
>> +			errno = EPIPE;
>> +		else
>> +			errno = EINVAL;
>> +	}
>> +
>> +	return result;
>> +}
>> +
>>   int mingw_access(const char *filename, int mode)
>>   {
>>   	wchar_t wfilename[MAX_PATH];
>
> Looks good. I tested the patch, and it fixes the failure exposed by
> t5571.11.
>
> Acked-by: Johannes Sixt <j6t@kdbg.org>
>
> Thanks!

Thanks, both.