[PATCH v2 1/2] storage-daemon: report unexpected arguments on the fly

Paolo Bonzini posted 2 patches 4 years, 11 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>
[PATCH v2 1/2] storage-daemon: report unexpected arguments on the fly
Posted by Paolo Bonzini 4 years, 11 months ago
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(-)

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[])
-- 
2.26.2



Re: [PATCH v2 1/2] storage-daemon: report unexpected arguments on the fly
Posted by Eric Blake 4 years, 11 months ago
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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH v2 1/2] storage-daemon: report unexpected arguments on the fly
Posted by Paolo Bonzini 4 years, 11 months ago
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


Re: [PATCH v2 1/2] storage-daemon: report unexpected arguments on the fly
Posted by Markus Armbruster 4 years, 11 months ago
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.