Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation
"brian m. carlson" 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.
Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation
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. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation
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
Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation
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?
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
[PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation
From: Karsten Blees
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
Signed-off-by: Johannes Schindelin
---
compat/mingw.c | 31 +--
1 file changed, 21 insertions(+), 10 deletions(-)
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)
--
gitgitgadget
