[PATCH v2 0/2] storage-daemon: include current command line option in the errors

[PATCH v2 0/2] storage-daemon: include current command line option in the errors

[Paolo Bonzini]

Use the location management facilities that the emulator uses, so that
the current command line option appears in the error message.

Before:

  $ storage-daemon/qemu-storage-daemon --nbd key..=
  qemu-storage-daemon: Invalid parameter 'key..'

After:

  $ storage-daemon/qemu-storage-daemon --nbd key..=
  qemu-storage-daemon: --nbd key..=: Invalid parameter 'key..'

The first patch tweaks the command line parsing so that argv is
not reordered by getopt_long and optind is only advanced by one
option+argument on every call to getopt_long.  This is required
by loc_set_cmdline.

Paolo Bonzini (2):
  storage-daemon: report unexpected arguments on the fly
  storage-daemon: include current command line option in the errors

 storage-daemon/qemu-storage-daemon.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

-- 
2.26.2

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

[Paolo Bonzini]

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

[PATCH v2 2/2] storage-daemon: include current command line option in the errors

[Paolo Bonzini]

Use the location management facilities that the emulator uses, so that
the current command line option appears in the error message.

Before:

  $ storage-daemon/qemu-storage-daemon --nbd key..=
  qemu-storage-daemon: Invalid parameter 'key..'

After:

  $ storage-daemon/qemu-storage-daemon --nbd key..=
  qemu-storage-daemon: --nbd key..=: Invalid parameter 'key..'

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 storage-daemon/qemu-storage-daemon.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 9aa82e7d96..78ddf619d4 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -152,6 +152,20 @@ static void init_qmp_commands(void)
                          qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
 }
 
+static int getopt_set_loc(int argc, char **argv, const char *optstring,
+                          const struct option *longopts)
+{
+    int c, save_index;
+
+    optarg = NULL;
+    save_index = optind;
+    c = getopt_long(argc, argv, optstring, longopts, NULL);
+    if (optarg) {
+        loc_set_cmdline(argv, save_index, MAX(1, optind - save_index));
+    }
+    return c;
+}
+
 static void process_options(int argc, char *argv[])
 {
     int c;
@@ -174,7 +188,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_set_loc(argc, argv, "-hT:V", long_options)) != -1) {
         switch (c) {
         case '?':
             exit(EXIT_FAILURE);
@@ -275,12 +289,13 @@ static void process_options(int argc, char *argv[])
                 break;
             }
         case 1:
-            error_report("Unexpected argument: %s", optarg);
+            error_report("Unexpected argument");
             exit(EXIT_FAILURE);
         default:
             g_assert_not_reached();
         }
     }
+    loc_set_none();
 }
 
 int main(int argc, char *argv[])
-- 
2.26.2

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

[Eric Blake]

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

[Paolo Bonzini]

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 0/2] storage-daemon: include current command line option in the errors

[Kevin Wolf]

Am 01.03.2021 um 16:28 hat Paolo Bonzini geschrieben:
> Use the location management facilities that the emulator uses, so that
> the current command line option appears in the error message.
> 
> Before:
> 
>   $ storage-daemon/qemu-storage-daemon --nbd key..=
>   qemu-storage-daemon: Invalid parameter 'key..'
> 
> After:
> 
>   $ storage-daemon/qemu-storage-daemon --nbd key..=
>   qemu-storage-daemon: --nbd key..=: Invalid parameter 'key..'
> 
> The first patch tweaks the command line parsing so that argv is
> not reordered by getopt_long and optind is only advanced by one
> option+argument on every call to getopt_long.  This is required
> by loc_set_cmdline.

Thanks, very useful to know about the "-" switch in getopts.

Applied to the block branch.

Kevin

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

[Markus Armbruster]

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.

Re: [PATCH v2 2/2] storage-daemon: include current command line option in the errors

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Use the location management facilities that the emulator uses, so that
> the current command line option appears in the error message.
>
> Before:
>
>   $ storage-daemon/qemu-storage-daemon --nbd key..=
>   qemu-storage-daemon: Invalid parameter 'key..'
>
> After:
>
>   $ storage-daemon/qemu-storage-daemon --nbd key..=
>   qemu-storage-daemon: --nbd key..=: Invalid parameter 'key..'
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I have a similar patch in an unfinished branch.  You win :)

> ---
>  storage-daemon/qemu-storage-daemon.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
> index 9aa82e7d96..78ddf619d4 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -152,6 +152,20 @@ static void init_qmp_commands(void)
>                           qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
>  }
>  
> +static int getopt_set_loc(int argc, char **argv, const char *optstring,
> +                          const struct option *longopts)
> +{
> +    int c, save_index;
> +
> +    optarg = NULL;
> +    save_index = optind;
> +    c = getopt_long(argc, argv, optstring, longopts, NULL);
> +    if (optarg) {
> +        loc_set_cmdline(argv, save_index, MAX(1, optind - save_index));
> +    }
> +    return c;
> +}
> +

I think this function is more widely applicable:

    $ git-grep -l getopt_long | xargs grep -l error_report
    qemu-img.c
    qemu-io.c
    qemu-nbd.c
    scsi/qemu-pr-helper.c
    storage-daemon/qemu-storage-daemon.c

>  static void process_options(int argc, char *argv[])
>  {
>      int c;
> @@ -174,7 +188,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_set_loc(argc, argv, "-hT:V", long_options)) != -1) {
>          switch (c) {
>          case '?':
>              exit(EXIT_FAILURE);
> @@ -275,12 +289,13 @@ static void process_options(int argc, char *argv[])
>                  break;
>              }
>          case 1:
> -            error_report("Unexpected argument: %s", optarg);
> +            error_report("Unexpected argument");
>              exit(EXIT_FAILURE);
>          default:
>              g_assert_not_reached();
>          }
>      }
> +    loc_set_none();
>  }
>  
>  int main(int argc, char *argv[])