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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 24, 2018, 07:37 UTC
Message-ID: <nycvar.QRO.7.76.6.1810240927520.4546@tvgsbejvaqbjf.bet>
In-Reply-To: <20181024022024.GE6119@genre.crustytoothpaste.net>

Hi brian,

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

Show 19 quoted lines

> 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