[Qemu-devel] [PATCH v2 0/3] qemu-img: improve qemu-img getopt error messages

[Qemu-devel] [PATCH v2 0/3] qemu-img: improve qemu-img getopt error messages

[Stefan Hajnoczi]

v2:
 * Print short help to avoid obscuring error messages [Max]

This series improves getopt error messages.  Unrecognized global options were
skipped rather than causing qemu-img to exit as expected.  Also avoid printing
the full help text because it obscures the actual error message.

Stefan Hajnoczi (3):
  qemu-img: show help for invalid global options
  qemu-img: fix switch indentation in img_amend()
  qemu-img: print short help on getopt failure

 qemu-img.c | 196 ++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 137 insertions(+), 59 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/3] qemu-img: show help for invalid global options

[Stefan Hajnoczi]

The qemu-img sub-command executes regardless of invalid global options:

  $ qemu-img --foo info test.img
  qemu-img: unrecognized option '--foo'
  image: test.img
  ...

The unrecognized option warning may be missed by the user.  This can
hide incorrect command-lines in scripts and confuse users.

This patch prints the help information and terminates instead of
executing the sub-command.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-img.c b/qemu-img.c
index 98b836b..ce293a4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4339,6 +4339,7 @@ int main(int argc, char **argv)
     while ((c = getopt_long(argc, argv, "+hVT:", long_options, NULL)) != -1) {
         switch (c) {
         case 'h':
+        case '?':
             help();
             return 0;
         case 'V':
-- 
2.9.3

[Qemu-devel] [PATCH v2 2/3] qemu-img: fix switch indentation in img_amend()

[Stefan Hajnoczi]

QEMU coding style indents 'case' to the same level as the 'switch'
statement:

  switch (foo) {
  case 1:

Fix this coding style violation so checkpatch.pl doesn't complain about
the next patch.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 82 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ce293a4..c7ffabb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3500,47 +3500,47 @@ static int img_amend(int argc, char **argv)
         }
 
         switch (c) {
-            case 'h':
-            case '?':
-                help();
-                break;
-            case 'o':
-                if (!is_valid_option_list(optarg)) {
-                    error_report("Invalid option list: %s", optarg);
-                    ret = -1;
-                    goto out_no_progress;
-                }
-                if (!options) {
-                    options = g_strdup(optarg);
-                } else {
-                    char *old_options = options;
-                    options = g_strdup_printf("%s,%s", options, optarg);
-                    g_free(old_options);
-                }
-                break;
-            case 'f':
-                fmt = optarg;
-                break;
-            case 't':
-                cache = optarg;
-                break;
-            case 'p':
-                progress = true;
-                break;
-            case 'q':
-                quiet = true;
-                break;
-            case OPTION_OBJECT:
-                opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                               optarg, true);
-                if (!opts) {
-                    ret = -1;
-                    goto out_no_progress;
-                }
-                break;
-            case OPTION_IMAGE_OPTS:
-                image_opts = true;
-                break;
+        case 'h':
+        case '?':
+            help();
+            break;
+        case 'o':
+            if (!is_valid_option_list(optarg)) {
+                error_report("Invalid option list: %s", optarg);
+                ret = -1;
+                goto out_no_progress;
+            }
+            if (!options) {
+                options = g_strdup(optarg);
+            } else {
+                char *old_options = options;
+                options = g_strdup_printf("%s,%s", options, optarg);
+                g_free(old_options);
+            }
+            break;
+        case 'f':
+            fmt = optarg;
+            break;
+        case 't':
+            cache = optarg;
+            break;
+        case 'p':
+            progress = true;
+            break;
+        case 'q':
+            quiet = true;
+            break;
+        case OPTION_OBJECT:
+            opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                           optarg, true);
+            if (!opts) {
+                ret = -1;
+                goto out_no_progress;
+            }
+            break;
+        case OPTION_IMAGE_OPTS:
+            image_opts = true;
+            break;
         }
     }
 
-- 
2.9.3

[Qemu-devel] [PATCH v2 3/3] qemu-img: print short help on getopt failure

[Stefan Hajnoczi]

Printing the full help output obscures the error message for an invalid
command-line option or missing argument.

Before this patch:

  $ ./qemu-img --foo
  ...pages of output...

After this patch:

  $ ./qemu-img --foo
  qemu-img: unrecognized option '--foo'
  Try 'qemu-img --help' for more information

This patch adds the getopt ':' character so that it can distinguish
between missing arguments and unrecognized options.  This helps provide
more detailed error messages.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 97 insertions(+), 20 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index c7ffabb..b220cf7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -88,6 +88,16 @@ static void QEMU_NORETURN GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...)
     exit(EXIT_FAILURE);
 }
 
+static void QEMU_NORETURN missing_argument(const char *option)
+{
+    error_exit("missing argument for option '%s'", option);
+}
+
+static void QEMU_NORETURN unrecognized_option(const char *option)
+{
+    error_exit("unrecognized option '%s'", option);
+}
+
 /* Please keep in synch with qemu-img.texi */
 static void QEMU_NORETURN help(void)
 {
@@ -406,13 +416,18 @@ static int img_create(int argc, char **argv)
             {"object", required_argument, 0, OPTION_OBJECT},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "F:b:f:he6o:q",
+        c = getopt_long(argc, argv, ":F:b:f:he6o:q",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -651,13 +666,18 @@ static int img_check(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:r:T:q",
+        c = getopt_long(argc, argv, ":hf:r:T:q",
                         long_options, &option_index);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -855,13 +875,18 @@ static int img_commit(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:ht:b:dpq",
+        c = getopt_long(argc, argv, ":f:ht:b:dpq",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -1190,13 +1215,18 @@ static int img_compare(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:T:pqs",
+        c = getopt_long(argc, argv, ":hf:F:T:pqs",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch (c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -1926,13 +1956,18 @@ static int img_convert(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qnm:W",
+        c = getopt_long(argc, argv, ":hf:O:B:ce6o:s:l:S:pt:T:qnm:W",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -2502,13 +2537,18 @@ static int img_info(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, ":f:h",
                         long_options, &option_index);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -2713,13 +2753,18 @@ static int img_map(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, ":f:h",
                         long_options, &option_index);
         if (c == -1) {
             break;
         }
         switch (c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -2835,13 +2880,18 @@ static int img_snapshot(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "la:c:d:hq",
+        c = getopt_long(argc, argv, ":la:c:d:hq",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             return 0;
@@ -2988,13 +3038,18 @@ static int img_rebase(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:b:upt:T:q",
+        c = getopt_long(argc, argv, ":hf:F:b:upt:T:q",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             return 0;
@@ -3355,13 +3410,18 @@ static int img_resize(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:hq",
+        c = getopt_long(argc, argv, ":f:hq",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -3493,15 +3553,20 @@ static int img_amend(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "ho:f:t:pq",
+        c = getopt_long(argc, argv, ":ho:f:t:pq",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
 
         switch (c) {
-        case 'h':
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
+        case 'h':
             help();
             break;
         case 'o':
@@ -3759,14 +3824,19 @@ static int img_bench(int argc, char **argv)
             {"no-drain", no_argument, 0, OPTION_NO_DRAIN},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hc:d:f:no:qs:S:t:w", long_options, NULL);
+        c = getopt_long(argc, argv, ":hc:d:f:no:qs:S:t:w", long_options, NULL);
         if (c == -1) {
             break;
         }
 
         switch (c) {
-        case 'h':
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
+        case 'h':
             help();
             break;
         case 'c':
@@ -4093,7 +4163,7 @@ static int img_dd(int argc, char **argv)
         { 0, 0, 0, 0 }
     };
 
-    while ((c = getopt_long(argc, argv, "hf:O:", long_options, NULL))) {
+    while ((c = getopt_long(argc, argv, ":hf:O:", long_options, NULL))) {
         if (c == EOF) {
             break;
         }
@@ -4104,10 +4174,12 @@ static int img_dd(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
-            error_report("Try 'qemu-img --help' for more information.");
-            ret = -1;
-            goto out;
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -4336,10 +4408,15 @@ int main(int argc, char **argv)
     qemu_add_opts(&qemu_source_opts);
     qemu_add_opts(&qemu_trace_opts);
 
-    while ((c = getopt_long(argc, argv, "+hVT:", long_options, NULL)) != -1) {
+    while ((c = getopt_long(argc, argv, "+:hVT:", long_options, NULL)) != -1) {
         switch (c) {
-        case 'h':
+        case ':':
+            missing_argument(argv[optind - 1]);
+            return 0;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            return 0;
+        case 'h':
             help();
             return 0;
         case 'V':
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/3] qemu-img: fix switch indentation in img_amend()

[Philippe Mathieu-Daudé]

On 03/17/2017 07:45 AM, Stefan Hajnoczi wrote:
> QEMU coding style indents 'case' to the same level as the 'switch'
> statement:
>
>   switch (foo) {
>   case 1:
>
> Fix this coding style violation so checkpatch.pl doesn't complain about
> the next patch.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  qemu-img.c | 82 +++++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index ce293a4..c7ffabb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3500,47 +3500,47 @@ static int img_amend(int argc, char **argv)
>          }
>
>          switch (c) {
> -            case 'h':
> -            case '?':
> -                help();
> -                break;
> -            case 'o':
> -                if (!is_valid_option_list(optarg)) {
> -                    error_report("Invalid option list: %s", optarg);
> -                    ret = -1;
> -                    goto out_no_progress;
> -                }
> -                if (!options) {
> -                    options = g_strdup(optarg);
> -                } else {
> -                    char *old_options = options;
> -                    options = g_strdup_printf("%s,%s", options, optarg);
> -                    g_free(old_options);
> -                }
> -                break;
> -            case 'f':
> -                fmt = optarg;
> -                break;
> -            case 't':
> -                cache = optarg;
> -                break;
> -            case 'p':
> -                progress = true;
> -                break;
> -            case 'q':
> -                quiet = true;
> -                break;
> -            case OPTION_OBJECT:
> -                opts = qemu_opts_parse_noisily(&qemu_object_opts,
> -                                               optarg, true);
> -                if (!opts) {
> -                    ret = -1;
> -                    goto out_no_progress;
> -                }
> -                break;
> -            case OPTION_IMAGE_OPTS:
> -                image_opts = true;
> -                break;
> +        case 'h':
> +        case '?':
> +            help();
> +            break;
> +        case 'o':
> +            if (!is_valid_option_list(optarg)) {
> +                error_report("Invalid option list: %s", optarg);
> +                ret = -1;
> +                goto out_no_progress;
> +            }
> +            if (!options) {
> +                options = g_strdup(optarg);
> +            } else {
> +                char *old_options = options;
> +                options = g_strdup_printf("%s,%s", options, optarg);
> +                g_free(old_options);
> +            }
> +            break;
> +        case 'f':
> +            fmt = optarg;
> +            break;
> +        case 't':
> +            cache = optarg;
> +            break;
> +        case 'p':
> +            progress = true;
> +            break;
> +        case 'q':
> +            quiet = true;
> +            break;
> +        case OPTION_OBJECT:
> +            opts = qemu_opts_parse_noisily(&qemu_object_opts,
> +                                           optarg, true);
> +            if (!opts) {
> +                ret = -1;
> +                goto out_no_progress;
> +            }
> +            break;
> +        case OPTION_IMAGE_OPTS:
> +            image_opts = true;
> +            break;
>          }
>      }
>
>

Re: [Qemu-devel] [PATCH v2 3/3] qemu-img: print short help on getopt failure

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

On 17.03.2017 11:45, Stefan Hajnoczi wrote:
> Printing the full help output obscures the error message for an invalid
> command-line option or missing argument.
> 
> Before this patch:
> 
>   $ ./qemu-img --foo
>   ...pages of output...
> 
> After this patch:
> 
>   $ ./qemu-img --foo
>   qemu-img: unrecognized option '--foo'
>   Try 'qemu-img --help' for more information
> 
> This patch adds the getopt ':' character so that it can distinguish
> between missing arguments and unrecognized options.  This helps provide
> more detailed error messages.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-img.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 97 insertions(+), 20 deletions(-)

Uh, right, this was pretty much pre-existing. Then even more so thanks
for this patch. :-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 0/3] qemu-img: improve qemu-img getopt error messages

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On 17.03.2017 11:45, Stefan Hajnoczi wrote:
> v2:
>  * Print short help to avoid obscuring error messages [Max]
> 
> This series improves getopt error messages.  Unrecognized global options were
> skipped rather than causing qemu-img to exit as expected.  Also avoid printing
> the full help text because it obscures the actual error message.
> 
> Stefan Hajnoczi (3):
>   qemu-img: show help for invalid global options
>   qemu-img: fix switch indentation in img_amend()
>   qemu-img: print short help on getopt failure
> 
>  qemu-img.c | 196 ++++++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 137 insertions(+), 59 deletions(-)

Thanks, applied to my block branch:

https://github.com/XanClic/qemu/commits/block

(Assuming that patches 2 and 3 will be fine even for rc2...)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/3] qemu-img: print short help on getopt failure

[Kashyap Chamarthy]

On Fri, Mar 17, 2017 at 06:45:41PM +0800, Stefan Hajnoczi wrote:
> Printing the full help output obscures the error message for an invalid
> command-line option or missing argument.
> 
> Before this patch:
> 
>   $ ./qemu-img --foo
>   ...pages of output...

That gives me:

    $ qemu-img --foo
    qemu-img: unrecognized option '--foo'

I think you meant any one of the `qemu-img create|convert|... --foo`:

    $ qemu-img create --foo
    [...]

Which _does_ give "pages of output".

Just noting if you want to tweak the commit message.

> After this patch:
> 
>   $ ./qemu-img --foo
>   qemu-img: unrecognized option '--foo'
>   Try 'qemu-img --help' for more information
> 

[...]

-- 
/kashyap

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/3] qemu-img: print short help on getopt failure

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

On 21.03.2017 23:15, Kashyap Chamarthy wrote:
> On Fri, Mar 17, 2017 at 06:45:41PM +0800, Stefan Hajnoczi wrote:
>> Printing the full help output obscures the error message for an invalid
>> command-line option or missing argument.
>>
>> Before this patch:
>>
>>   $ ./qemu-img --foo
>>   ...pages of output...
> 
> That gives me:
> 
>     $ qemu-img --foo
>     qemu-img: unrecognized option '--foo'

Yes, before this series. But not before this patch (i.e. after the first
two patches of this series).

Max

> 
> I think you meant any one of the `qemu-img create|convert|... --foo`:
> 
>     $ qemu-img create --foo
>     [...]
> 
> Which _does_ give "pages of output".
> 
> Just noting if you want to tweak the commit message.
> 
>> After this patch:
>>
>>   $ ./qemu-img --foo
>>   qemu-img: unrecognized option '--foo'
>>   Try 'qemu-img --help' for more information
>>
> 
> [...]
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]