diff mbox series

[2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

Message ID f2ce9bdc01892b514f75c6c25c3393765593b1ca.1540290197.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Use nanosecond-precision file times on Windows | expand

Commit Message

Linus Arver via GitGitGadget Oct. 23, 2018, 10:23 a.m. UTC 
From: Karsten Blees <blees@dcon.de>

fstat() is the only stat-related CRT function for which we don't have a
full replacement yet (and thus the only reason to stick with MSVCRT's
'struct stat' definition).

Fully implement fstat(), in preparation of implementing a POSIX 2013
compatible 'struct stat' with nanosecond-precision file times.

This allows us also to implement some clever code to handle pipes and
character devices in our own way.

Signed-off-by: Karsten Blees <blees@dcon.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

Comments

brian m. carlson Oct. 24, 2018, 2:20 a.m. UTC | #1 
On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote:
> -	if (!get_file_info_by_handle(fh, buf))
> +	case FILE_TYPE_CHAR:
> +	case FILE_TYPE_PIPE:
> +		/* initialize stat fields */
> +		memset(buf, 0, sizeof(*buf));
> +		buf->st_nlink = 1;
> +
> +		if (type == FILE_TYPE_CHAR) {
> +			buf->st_mode = _S_IFCHR;
> +		} else {
> +			buf->st_mode = _S_IFIFO;
> +			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
> +				buf->st_size = avail;

These lines strike me as a bit odd.  As far as I'm aware, Unix systems
don't return anything useful in this field when calling fstat on a pipe.
Is there a reason we fill this in on Windows?  If so, could the commit
message explain what that is?
Johannes Schindelin Oct. 24, 2018, 7:37 a.m. UTC | #2 
Hi brian,

On Wed, 24 Oct 2018, brian m. carlson wrote:

> On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote:
> > -	if (!get_file_info_by_handle(fh, buf))
> > +	case FILE_TYPE_CHAR:
> > +	case FILE_TYPE_PIPE:
> > +		/* initialize stat fields */
> > +		memset(buf, 0, sizeof(*buf));
> > +		buf->st_nlink = 1;
> > +
> > +		if (type == FILE_TYPE_CHAR) {
> > +			buf->st_mode = _S_IFCHR;
> > +		} else {
> > +			buf->st_mode = _S_IFIFO;
> > +			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
> > +				buf->st_size = avail;
> 
> These lines strike me as a bit odd.  As far as I'm aware, Unix systems
> don't return anything useful in this field when calling fstat on a pipe.
> Is there a reason we fill this in on Windows?  If so, could the commit
> message explain what that is?

AFAICT the idea was to imitate MSVCRT's fstat() in these cases.

But a quick web search suggests that you are right:
https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any
official documentation talking about fstat() and pipes, but I trust Alan
to know their stuff).

Do note, please, that according to the issue described in that link, at
least *some* glibc/Linux combinations behave in exactly the way this patch
implements it.

At this point, I am wary of changing this, too, as the code in question
has been in production (read: tested thoroughly) in the current form for
*years*, and I am really loathe to introduce a bug where even
Windows-specific code in compat/ might rely on this behavior. (And no, I
do not trust our test suite to find all of those use cases.)

Ciao,
Dscho
brian m. carlson Oct. 24, 2018, 10:40 p.m. UTC | #3 
On Wed, Oct 24, 2018 at 09:37:43AM +0200, Johannes Schindelin wrote:
> Hi brian,
> 
> On Wed, 24 Oct 2018, brian m. carlson wrote:
> > These lines strike me as a bit odd.  As far as I'm aware, Unix systems
> > don't return anything useful in this field when calling fstat on a pipe.
> > Is there a reason we fill this in on Windows?  If so, could the commit
> > message explain what that is?
> 
> AFAICT the idea was to imitate MSVCRT's fstat() in these cases.
> 
> But a quick web search suggests that you are right:
> https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any
> official documentation talking about fstat() and pipes, but I trust Alan
> to know their stuff).

Yeah, that behavior is quite old.  I'm surprised that Linux ever did
that.

> Do note, please, that according to the issue described in that link, at
> least *some* glibc/Linux combinations behave in exactly the way this patch
> implements it.
> 
> At this point, I am wary of changing this, too, as the code in question
> has been in production (read: tested thoroughly) in the current form for
> *years*, and I am really loathe to introduce a bug where even
> Windows-specific code in compat/ might rely on this behavior. (And no, I
> do not trust our test suite to find all of those use cases.)

I don't feel strongly either way.  I feel confident the rest of Git
doesn't use that field, so I don't see any downsides to keeping it other
than the slight overhead of populating it.  I just thought I'd ask in
case there was something important I was missing.
Junio C Hamano Oct. 25, 2018, 9:35 a.m. UTC | #4 
"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> Yeah, that behavior is quite old.  I'm surprised that Linux ever did
> that.
> ...
> I don't feel strongly either way.  I feel confident the rest of Git
> doesn't use that field, so I don't see any downsides to keeping it other
> than the slight overhead of populating it.  I just thought I'd ask in
> case there was something important I was missing.

OK, I'd consider that this part of the review settled for taking the
patch as-is.  Let's mark the topic for merging to 'next' soonish in
the what's cooking report.

Thanks.
diff mbox series

Patch

diff --git a/compat/mingw.c b/compat/mingw.c
index d2e7d86db..07fc0b79a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -771,20 +771,31 @@  int mingw_stat(const char *file_name, struct stat *buf)
 int mingw_fstat(int fd, struct stat *buf)
 {
 	HANDLE fh = (HANDLE)_get_osfhandle(fd);
+	DWORD avail, type = GetFileType(fh) & ~FILE_TYPE_REMOTE;
 
-	if (fh == INVALID_HANDLE_VALUE) {
-		errno = EBADF;
-		return -1;
-	}
-	/* direct non-file handles to MS's fstat() */
-	if (GetFileType(fh) != FILE_TYPE_DISK)
-		return _fstati64(fd, buf);
+	switch (type) {
+	case FILE_TYPE_DISK:
+		return get_file_info_by_handle(fh, buf);
 
-	if (!get_file_info_by_handle(fh, buf))
+	case FILE_TYPE_CHAR:
+	case FILE_TYPE_PIPE:
+		/* initialize stat fields */
+		memset(buf, 0, sizeof(*buf));
+		buf->st_nlink = 1;
+
+		if (type == FILE_TYPE_CHAR) {
+			buf->st_mode = _S_IFCHR;
+		} else {
+			buf->st_mode = _S_IFIFO;
+			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
+				buf->st_size = avail;
+		}
 		return 0;
 
-	errno = EBADF;
-	return -1;
+	default:
+		errno = EBADF;
+		return -1;
+	}
 }
 
 static inline void time_t_to_filetime(time_t t, FILETIME *ft)