I spent some time tidying up the patch and adding a more detailed commit message.
-- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From 2525e6aed066fe8eafdaab0d33c8c5df055c7e09 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Fri, 7 Jul 2023 20:00:47 -0700 Subject: [PATCH v5 1/1] Teach in-tree getopt_long() to move non-options to the end of argv. Unlike the other implementations of getopt_long() I could find, the in-tree implementation does not reorder non-options to the end of argv. Instead, it returns -1 as soon as the first non-option is found, even if there are other options listed afterwards. By moving non-options to the end of argv, getopt_long() can parse all specified options and return -1 when only non-options remain. This quirk is periodically missed by hackers (e.g., 869aa40a27, ffd398021c, and d9ddc50baf). This commit introduces the aforementioned non-option reordering behavior to the in-tree getopt_long() implementation. The only systems I'm aware of that use it are Windows and AIX, both of which have been tested. Special thanks to Noah Misch for his help validating behavior on AIX. Reviewed-by: Kyotaro Horiguchi, Michael Paquier Discussion: https://postgr.es/m/20230609232257.GA121461%40nathanxps13 --- src/bin/scripts/t/040_createuser.pl | 10 ++-- src/port/getopt_long.c | 79 ++++++++++++++++++++--------- 2 files changed, 60 insertions(+), 29 deletions(-) diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl index 9ca282181d..3290e30c88 100644 --- a/src/bin/scripts/t/040_createuser.pl +++ b/src/bin/scripts/t/040_createuser.pl @@ -42,9 +42,8 @@ $node->issues_sql_like( 'add a role as a member with admin option of the newly created role'); $node->issues_sql_like( [ - 'createuser', '-m', - 'regress_user3', '-m', - 'regress user #4', 'REGRESS_USER5' + 'createuser', 'REGRESS_USER5', '-m', 'regress_user3', + '-m', 'regress user #4' ], qr/statement: CREATE ROLE "REGRESS_USER5" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user3,"regress user #4";/, 'add a role as a member of the newly created role'); @@ -73,11 +72,14 @@ $node->issues_sql_like( qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/, '--role'); $node->issues_sql_like( - [ 'createuser', '--member-of', 'regress_user1', 'regress_user12' ], + [ 'createuser', 'regress_user12', '--member-of', 'regress_user1' ], qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/, '--member-of'); $node->command_fails([ 'createuser', 'regress_user1' ], 'fails if role already exists'); +$node->command_fails( + [ 'createuser', 'regress_user1', '-m', 'regress_user2', 'regress_user3' ], + 'fails for too many non-options'); done_testing(); diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c index c989276988..3fc2cf36e9 100644 --- a/src/port/getopt_long.c +++ b/src/port/getopt_long.c @@ -50,8 +50,9 @@ * This implementation does not use optreset. Instead, we guarantee that * it can be restarted on a new argv array after a previous call returned -1, * if the caller resets optind to 1 before the first call of the new series. - * (Internally, this means we must be sure to reset "place" to EMSG before - * returning -1.) + * + * Note that this routine reorders the pointers in argv (despite the const + * qualifier) so that all non-options will be at the end when -1 is returned. */ int getopt_long(int argc, char *const argv[], @@ -60,40 +61,68 @@ getopt_long(int argc, char *const argv[], { static char *place = EMSG; /* option letter processing */ char *oli; /* option letter list index */ + static int nonopt_start = -1; + static bool force_nonopt = false; if (!*place) { /* update scanning pointer */ - if (optind >= argc) + for (;;) { - place = EMSG; - return -1; - } + char **args = (char **) argv; - place = argv[optind]; + if (optind >= argc) + { + place = EMSG; + nonopt_start = -1; + force_nonopt = false; + return -1; + } - if (place[0] != '-') - { - place = EMSG; - return -1; - } + place = argv[optind]; + + /* + * Any string that starts with '-' but is not equivalent to '-' or + * '--' is considered an option. If we see an option, we can + * immediately break out of the loop since no argument reordering + * is required. Note that we treat '--' as the end of options and + * immediately force reordering for every subsequent argument. + * This reinstates the original order of all non-options (which + * includes everything after the '--') before we return. + */ + if (!force_nonopt && place[0] == '-' && place[1]) + { + if (place[1] != '-' || place[2]) + break; - place++; + optind++; + force_nonopt = true; + continue; + } - if (!*place) - { - /* treat "-" as not being an option */ - place = EMSG; - return -1; - } + /* + * If only non-options remain, return -1. Else, move the + * non-option to the end and try again. + */ + if (optind == nonopt_start) + { + place = EMSG; + nonopt_start = -1; + force_nonopt = false; + return -1; + } - if (place[0] == '-' && place[1] == '\0') - { - /* found "--", treat it as end of options */ - ++optind; - place = EMSG; - return -1; + for (int i = optind; i < argc - 1; i++) + args[i] = args[i + 1]; + args[argc - 1] = place; + + if (nonopt_start == -1) + nonopt_start = argc - 1; + else + nonopt_start--; } + place++; + if (place[0] == '-' && place[1]) { /* long option */ -- 2.25.1
