On 12/12/2025 9:57 PM, Thomas Munro wrote:
> On Sat, Dec 13, 2025 at 3:44 PM Tom Lane <[email protected]> wrote:
>> Thomas Munro <[email protected]> writes:
>>> Hearing nothing, pushed.
>>
>> fairywren is unimpressed:
>>
>> ../pgsql/src/test/modules/test_cloexec/test_cloexec.c: In function
>> 'run_parent_tests':
>> ../pgsql/src/test/modules/test_cloexec/test_cloexec.c:137:29: warning:
>> unused variable 'space_pos' [-Wunused-variable]
>> 137 | char *space_pos;
>> | ^~~~~~~~~
>
> The CI MinGW task also shows this warning, but it doesn't use -Werror.
> The separate CompileWarnings task does, being its purpose, and it
> includes a MinGW cross-build step, but that uses configure, and this
> test is built only by meson. That wasn't a great idea... we knew we
> were only dealing with Windows but forgot about MinGW, so I'll go and
> write a patch to fix that aspect later today so we're covered for
> warnings. I'll also think about whether it's worth checking for MinGW
> warnings in both assert and non-assert builds (as we do for regular
> Linux gcc/clang), and I'd also like to try to catch warnings from MSVC
> and had an idea for how to do that... I might also try to think about
> meson-vs-configure cross checks...
>
>> What is the point of that first snprintf(cmdline, ...), when its
>> result is guaranteed to be overwritten just below?
>>
>> I'm also dubious about using MAX_PATH here; see the commentary
>> about MAXPGPATH in pg_config_manual.h. Also, what's the point of
>> using MAX_PATH when the result is going to be transferred into
>> cmdline (with a hardwired size of 1024)?
>
> Fair points, I'll wait and see if Bryan is free to write a patch on
> Monday (US), and otherwise write one myself.
Thomas,
A sanity check would be appreciated after the somewhat embarrassing
sloppy code.
I removed the useless snprintf() call that was using GetCommandLine().
That was left in place when I moved to GetModuleFileName(). Also,
removed the unused 'space_pos' variable and the unneeded scope block. I
decided to just use 1024 for the exe_path size since that is what
cmdline is set to use. I also removed some self-evident comments that
were leftover from my practice of writing comments and then coding.
Apologies for the loss of time.
Thanks,
--
Bryan Green
EDB: https://www.enterprisedb.com
From 722d675cae77d046b8e53abbcf9f5218b375b180 Mon Sep 17 00:00:00 2001
From: Bryan Green <[email protected]>
Date: Fri, 12 Dec 2025 22:31:15 -0600
Subject: [PATCH v1] Clean up sloppy code in test_cloexec.c
The command line construction code in run_parent_tests() had several
issues:
1. A useless snprintf() call that built a command line using
GetCommandLine(), which was immediately overwritten by a second
snprintf() that correctly used GetModuleFileName() instead.
2. An unused variable 'space_pos' that was declared but never used.
3. An unnecessary scope block around the GetModuleFileName() call.
4. Inconsistent buffer sizing: exe_path used MAX_PATH while cmdline used
1024..
This appears to be code that was refactored when it was realized
GetCommandLine() wouldn't work correctly (it returns the full command
line with arguments, not just the executable path), but the old
approach was never fully removed.
Clean this up by:
- Removing the redundant first snprintf() call
- Removing the unused space_pos variable
- Removing the unnecessary scope block
- Using consistent 1024-byte buffer size for both exe_path and cmdline
- Adding a clearer comment explaining the purpose of the code
- Removed some simplistic, unneeded comments
---
src/test/modules/test_cloexec/test_cloexec.c | 34 +++-----------------
1 file changed, 5 insertions(+), 29 deletions(-)
diff --git a/src/test/modules/test_cloexec/test_cloexec.c
b/src/test/modules/test_cloexec/test_cloexec.c
index 9f64545168..a18d53a5d3 100644
--- a/src/test/modules/test_cloexec/test_cloexec.c
+++ b/src/test/modules/test_cloexec/test_cloexec.c
@@ -34,7 +34,6 @@ main(int argc, char *argv[])
char testfile1[MAXPGPATH];
char testfile2[MAXPGPATH];
- /* Windows-only test */
#ifndef WIN32
fprintf(stderr, "This test only runs on Windows\n");
return 0;
@@ -57,7 +56,6 @@ main(int argc, char *argv[])
run_parent_tests(testfile1, testfile2);
- /* Clean up test files */
unlink(testfile1);
unlink(testfile2);
@@ -78,6 +76,7 @@ run_parent_tests(const char *testfile1, const char *testfile2)
fd2;
HANDLE h1,
h2;
+ char exe_path[1024];
char cmdline[1024];
STARTUPINFO si;
PROCESS_INFORMATION pi;
@@ -106,7 +105,6 @@ run_parent_tests(const char *testfile1, const char
*testfile2)
exit(1);
}
- /* Get Windows HANDLEs from file descriptors */
h1 = (HANDLE) _get_osfhandle(fd1);
h2 = (HANDLE) _get_osfhandle(fd2);
@@ -121,25 +119,8 @@ run_parent_tests(const char *testfile1, const char
*testfile2)
printf("Parent: fd1=%d (O_CLOEXEC) -> HANDLE=%p\n", fd1, h1);
printf("Parent: fd2=%d (no O_CLOEXEC) -> HANDLE=%p\n", fd2, h2);
- /*
- * Spawn child process with bInheritHandles=TRUE, passing handle values
as
- * hex strings
- */
- snprintf(cmdline, sizeof(cmdline), "\"%s\" %p %p",
- GetCommandLine(), h1, h2);
-
- /*
- * Find the actual executable path by removing any arguments from
- * GetCommandLine().
- */
- {
- char exe_path[MAX_PATH];
- char *space_pos;
-
- GetModuleFileName(NULL, exe_path, sizeof(exe_path));
- snprintf(cmdline, sizeof(cmdline), "\"%s\" %p %p",
- exe_path, h1, h2);
- }
+ GetModuleFileName(NULL, exe_path, sizeof(exe_path));
+ snprintf(cmdline, sizeof(cmdline), "\"%s\" %p %p", exe_path, h1, h2);
memset(&si, 0, sizeof(si));
si.cb = sizeof(si);
@@ -167,7 +148,6 @@ run_parent_tests(const char *testfile1, const char
*testfile2)
printf("Parent: Waiting for child process...\n");
- /* Wait for child to complete */
WaitForSingleObject(pi.hProcess, INFINITE);
GetExitCodeProcess(pi.hProcess, &exit_code);
@@ -193,12 +173,9 @@ static void
run_child_tests(const char *handle1_str, const char *handle2_str)
{
#ifdef WIN32
- HANDLE h1,
- h2;
- bool h1_worked,
- h2_worked;
+ HANDLE h1, h2;
+ bool h1_worked, h2_worked;
- /* Parse handle values from hex strings */
if (sscanf(handle1_str, "%p", &h1) != 1 ||
sscanf(handle2_str, "%p", &h2) != 1)
{
@@ -209,7 +186,6 @@ run_child_tests(const char *handle1_str, const char
*handle2_str)
printf("Child: Received HANDLE1=%p (should fail - O_CLOEXEC)\n", h1);
printf("Child: Received HANDLE2=%p (should work - no O_CLOEXEC)\n", h2);
- /* Try to write to both handles */
h1_worked = try_write_to_handle(h1, "HANDLE1");
h2_worked = try_write_to_handle(h2, "HANDLE2");
--
2.52.0.windows.1
