On Wed, 22 Jul 2026 at 12:17, Japin Li <[email protected]> wrote: > Hi, Michael > > Thanks for reviewing the patch. > > <#secure method=pgpmime mode=sign> > On Wed, 22 Jul 2026 at 10:47, Michael Paquier <[email protected]> wrote: >> On Tue, Jul 21, 2026 at 06:16:27PM +0800, Japin Li wrote: >>> + place = EMSG; >>> + optind++; >>> + >>> if (optstring[0] == ':') >>> return BADARG; >>> >>> @@ -153,9 +156,6 @@ retry: >>> "%s: option requires an argument -- %s\n", >>> argv[0], place); >>> >>> - place = EMSG; >>> - optind++; >>> - >>> if (has_arg == required_argument) >>> return BADCH; >>> optarg = NULL; >> >> This also means that for long options we have an error message would >> now use an empty string instead of an option name all the time. That >> looks incorrect. > > Right. I was mistaken about the option name. Fixed in v2-0002. > >> >> You are claiming that this makes the short option path more >> consistent. That's true based on what your patch does, but it also >> looks to me like the short option area is already wrong in setting >> EMSG before we issue the error string "option requires an argument". >> So, to me, you are making the situation worse in some cases while >> claiming that it improves the situation in some other cases. >> >> Our implementation of getopt_long() is only used on Windows for MSVC, >> as far as I know. I'd also suggest to post one or more examples of >> how this changes the implementation behavior. You should be able to >> force your way through easily so as the Postgres getopt_long() >> implementation is used with a quick hack, just to prove your point, >> saving you the pain of deploying a WIN32 host.. (Like use a >> pg_getopt_long() and patch one of the binaries, whatever you prefer.) > > Here's a mini‑demo for getopt_long() in v2‑0001. Without v2‑0002, the output > is: > > $ ./getopt_long_test --log-filename > option './getopt_long_test' requires an argument > unrecognized option './getopt_long_test' > unrecognized option './getopt_long_test' > unrecognized option './getopt_long_test' > unrecognized option './getopt_long_test' > $ ./getopt_long_test -f > option '-f' requires an argument > > With v2‑0002 applied, the output is: > > $ ./getopt_long_test --log-filename > option '--log-filename' requires an argument > $ ./getopt_long_test -f > option '-f' requires an argument > >> -- >> Michael >
Sorry, I forgot to include the patches in my previous email. > -- > Regards, > Japin Li > ChengDu WenWu Information Technology Co., Ltd. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
>From a5b0c4eb3360e22fa66af2c039c69a63c09df04d Mon Sep 17 00:00:00 2001 From: Japin Li <[email protected]> Date: Wed, 22 Jul 2026 11:55:36 +0800 Subject: [PATCH v2 1/2] Add a binary to test getopt_long() --- src/bin/Makefile | 1 + src/bin/getopt_long_test/Makefile | 26 +++++++++++++++++++++ src/bin/getopt_long_test/getopt_long_test.c | 25 ++++++++++++++++++++ 3 files changed, 52 insertions(+) create mode 100644 src/bin/getopt_long_test/Makefile create mode 100644 src/bin/getopt_long_test/getopt_long_test.c diff --git a/src/bin/Makefile b/src/bin/Makefile index 538af88a523..b25305f9db5 100644 --- a/src/bin/Makefile +++ b/src/bin/Makefile @@ -14,6 +14,7 @@ top_builddir = ../.. include $(top_builddir)/src/Makefile.global SUBDIRS = \ + getopt_long_test \ initdb \ pg_amcheck \ pg_archivecleanup \ diff --git a/src/bin/getopt_long_test/Makefile b/src/bin/getopt_long_test/Makefile new file mode 100644 index 00000000000..f7de6ef872c --- /dev/null +++ b/src/bin/getopt_long_test/Makefile @@ -0,0 +1,26 @@ +PGFILEDESC = "getopt_long_test - test PostgreSQL getopt_long() function" +PGAPPICON = win32 + +subdir = src/bin/getopt_long_test +top_builddir = ../../.. +include $(top_builddir)/src/Makefile.global + +PG_CPPFLAGS += -I$(top_srcdir)/src/port + +OBJS = \ + $(WIN32RES) \ + getopt_long.o \ + getopt_long_test.o + +all: getopt_long_test + +getopt_long_test: $(OBJS) + $(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) + +getopt_long_test.o: getopt_long_test.c $(top_builddir)/src/Makefile.global + +getopt_long.c: + ln -s $(top_srcdir)/src/port/getopt_long.c . + +clean distclean: + rm -f getopt_long_test$(X) $(OBJS) getopt_long.c diff --git a/src/bin/getopt_long_test/getopt_long_test.c b/src/bin/getopt_long_test/getopt_long_test.c new file mode 100644 index 00000000000..4abd042bbaa --- /dev/null +++ b/src/bin/getopt_long_test/getopt_long_test.c @@ -0,0 +1,25 @@ + +#include "postgres.h" + +#include "getopt_long.h" + +int +main(int argc, char **argv) +{ + int c; + const char *shortopts = ":f:"; + struct option longopts[] = { + {"log-filename", required_argument, NULL, 'f'}, + {NULL, 0, NULL, 0}, + }; + + while ((c = getopt_long(argc, argv, shortopts, longopts, NULL)) != -1) + { + if (c == ':') + fprintf(stderr, "option '%s' requires an argument\n", argv[optind - 1]); + else if (c == '?') + fprintf(stderr, "unrecognized option '%s'\n", argv[optind - 1]); + } + + return 0; +} -- 2.53.0
>From b9ca9e706f26f90d680c8a69716acc8b1ba90d3d Mon Sep 17 00:00:00 2001 From: Japin Li <[email protected]> Date: Wed, 22 Jul 2026 11:59:41 +0800 Subject: [PATCH v2 2/2] Fix optind handling inconsistency in getopt_long() for missing argument In the long option error path for a missing required argument, the code previously checked optstring[0] == ':' and returned BADARG immediately without incrementing optind and setting place = EMSG. This left optind and place in an inconsistent state compared to the short option path, where these updates are performed before the check and return. --- src/port/getopt_long.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c index 20953db9db1..2e869fed58b 100644 --- a/src/port/getopt_long.c +++ b/src/port/getopt_long.c @@ -145,8 +145,12 @@ retry: } else { + optind++; if (optstring[0] == ':') + { + place = EMSG; return BADARG; + } if (opterr && has_arg == required_argument) fprintf(stderr, @@ -154,7 +158,6 @@ retry: argv[0], place); place = EMSG; - optind++; if (has_arg == required_argument) return BADCH; -- 2.53.0
