| Message ID | 20210301152844.291799-2-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | storage-daemon: include current command line option in the errors | expand |
On 3/1/21 9:28 AM, Paolo Bonzini wrote: > If the first character of optstring is '-', then each nonoption argv > element is handled as if it were the argument of an option with character > code 1. This removes the reordering of the argv array, and enables usage > of loc_set_cmdline to provide better error messages. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > storage-daemon/qemu-storage-daemon.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) Nice. The man page for 'getopt_long' is unclear whether setting POSIXLY_CORRECT in the environment would break this (that is, setting POSIXLY_CORRECT has the same effect as a leading '+'; but you can't have both leading '+' and leading '-' and when both are set, it is not clear which one wins). But that's a corner case that I don't think will ever bite us in real life. Reviewed-by: Eric Blake <eblake@redhat.com>
On 01/03/21 16:38, Eric Blake wrote: > On 3/1/21 9:28 AM, Paolo Bonzini wrote: >> If the first character of optstring is '-', then each nonoption argv >> element is handled as if it were the argument of an option with character >> code 1. This removes the reordering of the argv array, and enables usage >> of loc_set_cmdline to provide better error messages. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> storage-daemon/qemu-storage-daemon.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) > > Nice. The man page for 'getopt_long' is unclear whether setting > POSIXLY_CORRECT in the environment would break this It doesn't. In fact, with this patch the behavior is the same as for POSIXLY_CORRECT, though for unrelated reasons, and interestingly enough I think the POSIXLY_CORRECT behavior is an improvement for qemu-storage-daemon. Unpatched: $ qemu-storage-daemon foo --object iothread qemu-storage-daemon: Parameter 'id' is missing $ POSIXLY_CORRECT=1 qemu-storage-daemon foo --object iothread qemu-storage-daemon: Unexpected argument: foo Patched: $ storage-daemon/qemu-storage-daemon foo --object iothread qemu-storage-daemon: foo: Unexpected argument $ POSIXLY_CORRECT=1 storage-daemon/qemu-storage-daemon foo --object iothread qemu-storage-daemon: foo: Unexpected argument Paolo
Eric Blake <eblake@redhat.com> writes: > On 3/1/21 9:28 AM, Paolo Bonzini wrote: >> If the first character of optstring is '-', then each nonoption argv >> element is handled as if it were the argument of an option with character >> code 1. This removes the reordering of the argv array, and enables usage >> of loc_set_cmdline to provide better error messages. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> storage-daemon/qemu-storage-daemon.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) > > Nice. The man page for 'getopt_long' is unclear whether setting > POSIXLY_CORRECT in the environment would break this (that is, setting > POSIXLY_CORRECT has the same effect as a leading '+'; but you can't have > both leading '+' and leading '-' and when both are set, it is not clear > which one wins). But that's a corner case that I don't think will ever > bite us in real life. > > Reviewed-by: Eric Blake <eblake@redhat.com> I'd consider environment overruling the programmer's express intent a bug. GLibc's _getopt_initialize(): /* Determine how to handle the ordering of options and nonoptions. */ if (optstring[0] == '-') { d->__ordering = RETURN_IN_ORDER; ++optstring; } else if (optstring[0] == '+') { d->__ordering = REQUIRE_ORDER; ++optstring; } else if (posixly_correct || !!getenv ("POSIXLY_CORRECT")) d->__ordering = REQUIRE_ORDER; else d->__ordering = PERMUTE; No surprises here.
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index 9021a46b3a..9aa82e7d96 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -174,7 +174,7 @@ static void process_options(int argc, char *argv[]) * they are given on the command lines. This means that things must be * defined first before they can be referenced in another option. */ - while ((c = getopt_long(argc, argv, "hT:V", long_options, NULL)) != -1) { + while ((c = getopt_long(argc, argv, "-hT:V", long_options, NULL)) != -1) { switch (c) { case '?': exit(EXIT_FAILURE); @@ -275,14 +275,13 @@ static void process_options(int argc, char *argv[]) qobject_unref(args); break; } + case 1: + error_report("Unexpected argument: %s", optarg); + exit(EXIT_FAILURE); default: g_assert_not_reached(); } } - if (optind != argc) { - error_report("Unexpected argument: %s", argv[optind]); - exit(EXIT_FAILURE); - } } int main(int argc, char *argv[])
If the first character of optstring is '-', then each nonoption argv element is handled as if it were the argument of an option with character code 1. This removes the reordering of the argv array, and enables usage of loc_set_cmdline to provide better error messages. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- storage-daemon/qemu-storage-daemon.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)