public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: add non-option reordering to in-tree getopt_long
Date: Fri, 7 Jul 2023 20:52:24 -0700
Message-ID: <20230708035224.GA4042213@nathanxps13> (raw)
In-Reply-To: <20230621175624.GA882103@nathanxps13>
References: <20230616045828.GA972663@nathanxps13>
<[email protected]>
<20230616182847.GA71216@nathanxps13>
<[email protected]>
<20230621175624.GA882103@nathanxps13>
I spent some time tidying up the patch and adding a more detailed commit
message.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v5-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch (5.5K, ../20230708035224.GA4042213@nathanxps13/2-v5-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch)
download | inline diff:
From 2525e6aed066fe8eafdaab0d33c8c5df055c7e09 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
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
view thread (3+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: add non-option reordering to in-tree getopt_long
In-Reply-To: <20230708035224.GA4042213@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox