From: Johannes Sixt <j6t@kdbg.org>
Date: Wed, 23 Sep 2026 04:41:47 GMT
Subject: Re: [PATCH v3] compat/winansi: fix die_lasterr() argument formatting
Message-ID: <3e2befed-355b-4a82-af82-25dedaee0565@kdbg.org>
In-Reply-To: <20260921234756.77997-1-yqtian668@gmail.com>

Am 22.09.26 um 01:47 schrieb Yongqiang Tian:
> During WinANSI initialization, duplicate_handle() reports the handle
> when DuplicateHandle() fails. die_lasterr() collects the formatting
> arguments in a va_list, but passes that va_list to die_errno() as an
> ordinary variadic argument. die_errno() consequently formats part of
> the va_list representation instead of the supplied handle, producing
> an incorrect fatal message.
> 
> The helper also converts GetLastError() to errno, losing the exact
> Windows error code.
> 
> Remove die_lasterr() and report GetLastError() directly at its four
> call sites, following the existing Windows diagnostic style. This
> passes the handle to the formatter correctly and preserves the Windows
> error code. Keep the existing %li representation of the handle.
> 
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Yongqiang Tian <yqtian668@gmail.com>
> ---
> 
> Changes since v2:
> - Add Helped-by trailers for Johannes Sixt and René Scharfe.
> - Move build validation details below the separator.
> - No code changes.

This round looks very good now. I tested it and it works as desired.
Thanks! FWIW:

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

> 
> Validation (performed for v2; the code is unchanged):
> - Built compat/winansi.o with DEVELOPER=1 using MinGW GCC 13.
> - Built and linked the complete git.exe.
> 
>  compat/winansi.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 3ce1900939..088734a1df 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -436,15 +436,6 @@ static void winansi_exit(void)
>  	CloseHandle(hthread);
>  }
>  
> -static void die_lasterr(const char *fmt, ...)
> -{
> -	va_list params;
> -	va_start(params, fmt);
> -	errno = err_win_to_posix(GetLastError());
> -	die_errno(fmt, params);
> -	va_end(params);
> -}
> -
>  #undef dup2
>  int winansi_dup2(int oldfd, int newfd)
>  {
> @@ -462,8 +453,8 @@ static HANDLE duplicate_handle(HANDLE hnd)
>  	HANDLE hresult, hproc = GetCurrentProcess();
>  	if (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE,
>  			DUPLICATE_SAME_ACCESS))
> -		die_lasterr("DuplicateHandle(%li) failed",
> -			(long) (intptr_t) hnd);
> +		die("DuplicateHandle(%li) failed: %lu",
> +		    (long) (intptr_t) hnd, GetLastError());
>  	return hresult;
>  }
>  
> @@ -609,16 +600,16 @@ void winansi_init(void)
>  	hwrite = CreateNamedPipeW(name, PIPE_ACCESS_OUTBOUND,
>  		PIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL);
>  	if (hwrite == INVALID_HANDLE_VALUE)
> -		die_lasterr("CreateNamedPipe failed");
> +		die("CreateNamedPipe failed: %lu", GetLastError());
>  
>  	hread = CreateFileW(name, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL);
>  	if (hread == INVALID_HANDLE_VALUE)
> -		die_lasterr("CreateFile for named pipe failed");
> +		die("CreateFile for named pipe failed: %lu", GetLastError());
>  
>  	/* start console spool thread on the pipe's read end */
>  	hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);
>  	if (!hthread)
> -		die_lasterr("CreateThread(console_thread) failed");
> +		die("CreateThread(console_thread) failed: %lu", GetLastError());
>  
>  	/* schedule cleanup routine */
>  	if (atexit(winansi_exit))