On Sat, Dec 13, 2025 at 7:33 PM Tom Lane <[email protected]> wrote: > > Bryan Green <[email protected]> writes: > > 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. > > All good to my eye.
Thanks both. > > I decided to just use 1024 for the exe_path size since that is what > > cmdline is set to use. > > Personally I'd have gone the other way, say > > char exe_path[MAXPGPATH]; > char cmdline[MAXPGPATH + 100]; Done in this version. > > I also removed some self-evident comments that > > were leftover from my practice of writing comments and then coding. > > I think you went way overboard on removing "self-evident" comments. > Signposts as to what the code intends to do are pretty helpful IMO. > They do have to be accurate though, for instance this previous > comment: > > - * Find the actual executable path by removing any arguments from > - * GetCommandLine(). > > didn't seem to convey what the code was doing (which I neglected > to complain about before). Comments restored in the attached version of Bryan's patch. My earlier guess about the Makefile was wrong, and when I looked into it the actual problems were (1) that the CompilerWarnings task in CI runs make world-bin, which doesn't descend into src/test, and (2) that the test ifeq ($(PORTNAME), win32) was not satisfied due to make's rules for variable evaluation. I thought about how to fix that but realised that this is going to be much easier to maintain if it's not different on Unix, so here are some fixes in that direction. With just 0001 and 0002 applied, we'd have known about the compiler warning before commit, with a failure like this: https://cirrus-ci.com/task/5863371716689920 With 0003 applied on top, it's green and there are no warnings from either Windows task: https://cirrus-ci.com/build/4775547869331456 I also changed the comment style of some single-line comments. replaced the memset() with initializer syntax and ran pgindent which undid a change or two.
From b0767473504c0c313150ad47b531967879314fbf Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sun, 14 Dec 2025 12:30:24 +1300 Subject: [PATCH 1/3] ci: Check src/test in CompilerWarnings task. Since make world-bin doesn't visit src/test, CI was not checking for compiler warnings in test modules. Backpatch-through: 15, where CI began Discussion: https://postgr.es/m/1086088.1765593851%40sss.pgh.pa.us --- .cirrus.tasks.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml index 038d043d00e..29c7a6c0a07 100644 --- a/.cirrus.tasks.yml +++ b/.cirrus.tasks.yml @@ -944,6 +944,7 @@ task: CC="ccache gcc" CXX="ccache g++" CLANG="ccache clang" make -s -j${BUILD_JOBS} clean time make -s -j${BUILD_JOBS} world-bin + time make -C src/test -s -j${BUILD_JOBS} # gcc, cassert on, dtrace off always: @@ -955,6 +956,7 @@ task: CC="ccache gcc" CXX="ccache g++" CLANG="ccache clang" make -s -j${BUILD_JOBS} clean time make -s -j${BUILD_JOBS} world-bin + time make -C src/test -s -j${BUILD_JOBS} # clang, cassert off, dtrace off always: @@ -965,6 +967,7 @@ task: CC="ccache clang" CXX="ccache clang++" CLANG="ccache clang" make -s -j${BUILD_JOBS} clean time make -s -j${BUILD_JOBS} world-bin + time make -C src/test -s -j${BUILD_JOBS} # clang, cassert on, dtrace on always: @@ -977,6 +980,7 @@ task: CC="ccache clang" CXX="ccache clang++" CLANG="ccache clang" make -s -j${BUILD_JOBS} clean time make -s -j${BUILD_JOBS} world-bin + time make -C src/test -s -j${BUILD_JOBS} # cross-compile to windows always: @@ -989,6 +993,7 @@ task: CXX="ccache x86_64-w64-mingw32ucrt-g++" make -s -j${BUILD_JOBS} clean time make -s -j${BUILD_JOBS} world-bin + time make -C src/test -s -j${BUILD_JOBS} ### # Verify docs can be built -- 2.51.2
From 897d47122ef39f826eae3f3b7db8cb85e45d3c84 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sun, 14 Dec 2025 17:14:38 +1300 Subject: [PATCH 2/3] Fix Makefile used for test_cloexec. The previous test for ifeq ($(PORTNAME), win32) never succeeded, so only meson-based builds were building and testing our O_CLOEXEC emulation. Defect in commit c507ba55. That could probably be made to work, but it seems better to compile test_cloexec.c unconditionally and skip the test for non-Windows systems. Though the test is skipped on Unix and the executable wouldn't test anything if run (there is not much point in testing that the system O_CLOEXEC works on Unix), at least this way the configure-based build machinery is consistent, for ease of maintenance by Unix-based developers. Backpatch-through: 16, like commit c507ba55 --- src/test/modules/test_cloexec/Makefile | 16 ++++----------- src/test/modules/test_cloexec/test_cloexec.c | 21 +++++++++----------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/test/modules/test_cloexec/Makefile b/src/test/modules/test_cloexec/Makefile index 70d38575e26..94342319782 100644 --- a/src/test/modules/test_cloexec/Makefile +++ b/src/test/modules/test_cloexec/Makefile @@ -1,22 +1,14 @@ # src/test/modules/test_cloexec/Makefile -# This module is for Windows only -ifeq ($(PORTNAME),win32) - -MODULE_big = test_cloexec -OBJS = \ - $(WIN32RES) \ - test_cloexec.o - PGFILEDESC = "test_cloexec - test O_CLOEXEC flag handling" +PGAPPICON = win32 -# Build as a standalone program, not a shared library -PROGRAM = test_cloexec -override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS) +PROGRAM += test_cloexec +OBJS += $(WIN32RES) test_cloexec.o +NO_INSTALLCHECK = 1 TAP_TESTS = 1 -endif ifdef USE_PGXS PG_CONFIG = pg_config diff --git a/src/test/modules/test_cloexec/test_cloexec.c b/src/test/modules/test_cloexec/test_cloexec.c index 9f645451684..7ff6585d710 100644 --- a/src/test/modules/test_cloexec/test_cloexec.c +++ b/src/test/modules/test_cloexec/test_cloexec.c @@ -24,21 +24,22 @@ #include "common/file_utils.h" #include "port.h" +#ifdef WIN32 static void run_parent_tests(const char *testfile1, const char *testfile2); static void run_child_tests(const char *handle1_str, const char *handle2_str); static bool try_write_to_handle(HANDLE h, const char *label); +#endif int main(int argc, char *argv[]) { - char testfile1[MAXPGPATH]; - char testfile2[MAXPGPATH]; - - /* Windows-only test */ #ifndef WIN32 + /* Windows-only test */ fprintf(stderr, "This test only runs on Windows\n"); return 0; -#endif +#else + char testfile1[MAXPGPATH]; + char testfile2[MAXPGPATH]; if (argc == 3) { @@ -68,12 +69,13 @@ main(int argc, char *argv[]) fprintf(stderr, "Usage: %s [handle1_hex handle2_hex]\n", argv[0]); return 1; } +#endif } +#ifdef WIN32 static void run_parent_tests(const char *testfile1, const char *testfile2) { -#ifdef WIN32 int fd1, fd2; HANDLE h1, @@ -186,13 +188,11 @@ run_parent_tests(const char *testfile1, const char *testfile2) printf("Parent: FAILURE - O_CLOEXEC not working correctly\n"); exit(1); } -#endif } static void run_child_tests(const char *handle1_str, const char *handle2_str) { -#ifdef WIN32 HANDLE h1, h2; bool h1_worked, @@ -232,13 +232,11 @@ run_child_tests(const char *handle1_str, const char *handle2_str) printf("Child: Test FAILED - O_CLOEXEC not working correctly\n"); exit(1); } -#endif } static bool try_write_to_handle(HANDLE h, const char *label) { -#ifdef WIN32 const char *test_data = "test\n"; DWORD bytes_written; BOOL result; @@ -256,7 +254,6 @@ try_write_to_handle(HANDLE h, const char *label) label, GetLastError()); return false; } -#else return false; -#endif } +#endif -- 2.51.2
From 47a7e7406c0896e985435360013d99f0553dd607 Mon Sep 17 00:00:00 2001 From: Bryan Green <[email protected]> Date: Fri, 12 Dec 2025 22:31:15 -0600 Subject: [PATCH 3/3] 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 buitlt 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 Per code review from Tom Lane. Backpatch-through: 16, like commit c507ba55 Author: Bryan Green <[email protected]> Reviewed-by: Tom Lane <[email protected]> Reviewed-by: Thomas Munro <[email protected]> Discussion: https://postgr.es/m/1086088.1765593851%40sss.pgh.pa.us --- src/test/modules/test_cloexec/test_cloexec.c | 40 ++++++-------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/src/test/modules/test_cloexec/test_cloexec.c b/src/test/modules/test_cloexec/test_cloexec.c index 7ff6585d710..621a3cc02f5 100644 --- a/src/test/modules/test_cloexec/test_cloexec.c +++ b/src/test/modules/test_cloexec/test_cloexec.c @@ -80,16 +80,15 @@ run_parent_tests(const char *testfile1, const char *testfile2) fd2; HANDLE h1, h2; - char cmdline[1024]; - STARTUPINFO si; - PROCESS_INFORMATION pi; + char exe_path[MAXPGPATH]; + char cmdline[MAXPGPATH + 100]; + STARTUPINFO si = {.cb = sizeof(si)}; + PROCESS_INFORMATION pi = {0}; DWORD exit_code; printf("Parent: Opening test files...\n"); - /* - * Open first file WITH O_CLOEXEC - should NOT be inherited - */ + /* Open first file WITH O_CLOEXEC - should NOT be inherited */ fd1 = open(testfile1, O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, 0600); if (fd1 < 0) { @@ -97,9 +96,7 @@ run_parent_tests(const char *testfile1, const char *testfile2) exit(1); } - /* - * Open second file WITHOUT O_CLOEXEC - should be inherited - */ + /* Open second file WITHOUT O_CLOEXEC - should be inherited */ fd2 = open(testfile2, O_RDWR | O_CREAT | O_TRUNC, 0600); if (fd2 < 0) { @@ -123,29 +120,12 @@ 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(). + * GetCommandLine(), and add our new arguments. */ - { - 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); - } - - memset(&si, 0, sizeof(si)); - si.cb = sizeof(si); - memset(&pi, 0, sizeof(pi)); + GetModuleFileName(NULL, exe_path, sizeof(exe_path)); + snprintf(cmdline, sizeof(cmdline), "\"%s\" %p %p", exe_path, h1, h2); printf("Parent: Spawning child process...\n"); printf("Parent: Command line: %s\n", cmdline); @@ -182,7 +162,9 @@ run_parent_tests(const char *testfile1, const char *testfile2) printf("Parent: Child exit code: %lu\n", exit_code); if (exit_code == 0) + { printf("Parent: SUCCESS - O_CLOEXEC behavior verified\n"); + } else { printf("Parent: FAILURE - O_CLOEXEC not working correctly\n"); -- 2.51.2
