Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2019, 06:26 UTC
Message-ID: <xmqqfti41rz1.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqq1rtp2k01.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Just like on POSIX the value of errno is indeterminate after a
>> successful system call, the value of GetLastError() indeterminate after
>> a successful Windows API call. Therefore, the err_win_to_posix() would
>> not be able to point at a bogus caller reliably. For this reason, let's
>> consider the function as a simple error code translator, and then
>> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.
>
> OK, that makes sense.

Actually, I do not think it makes that much sense, especially in the context of this patch that claims to map success to 0 (assuming that no E_ANYTHING has the value of zero---I am not sure POSIX gives such a guarantee) "for good measure".

Even if Windows API makes the GetLastError() unusable after a success call (unlike POSIX, where errno is left alone), the API calls themselves would be signaling their own success or failure, right? So I would have imagined that any kosher caller would be doing this:

	if (SomeWinAPI() != SUCCESS) {
		errno = err_win_to_posix(GetLastError());
		... possibly other reactions to the error ...
	}

If using a value grabbed from GetLastError() after a successfull Windows API call is a wrong thing to do as you taught us in your message, then an unconditional

	SomeWinAPI();
	errno = err_win_to_posix(GetLastError());

would be wrong anyway, and touching errno unconditionally, when the previous call may have succeeded without checking, makes the pattern doubly wrong, no?

Having said that, I do not expect myself to be looking into and fixing anything in compat/*win*.[ch], so I do not care too deeply either way, but I thought that it would help keep the sanity of developers involved if we touched errno only upon a failure from an underlying system.

Thanks.