diff mbox

[v2,3/3] qemu-img: print short help on getopt failure

Message ID 20170317104541.28979-4-stefanha@redhat.com
State New
Headers show

Commit Message

Stefan Hajnoczi March 17, 2017, 10:45 a.m. UTC 
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(-)

Comments

Max Reitz March 18, 2017, 2:56 a.m. UTC | #1 
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>
Kashyap Chamarthy March 21, 2017, 10:15 p.m. UTC | #2 
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
> 

[...]
Max Reitz March 22, 2017, 2:12 p.m. UTC | #3 
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
>>
> 
> [...]
>
diff mbox

Patch

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':