diff mbox

[PULL,14/16] qemu-img: Improve error messages

Message ID 1398247491-13985-15-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf April 23, 2014, 10:04 a.m. UTC 
From: Fam Zheng <famz@redhat.com>

Previously, when there is a user error in argv parsing, qemu-img prints
help text and exits.

Add an error_exit function to print a helpful error message and a hint
to run 'qemu-img --help' for more information.

As a bonus, "qemu-img <cmd> --help" now has a more reasonable exit code
0.

In the future the help text should be split by sub command, and only
print the information for the specified command.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 74 ++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 28 deletions(-)

Comments

Jeff Cody April 25, 2014, 6:18 p.m. UTC | #1 
On Wed, Apr 23, 2014 at 12:04:49PM +0200, Kevin Wolf wrote:
> From: Fam Zheng <famz@redhat.com>
> 
> Previously, when there is a user error in argv parsing, qemu-img prints
> help text and exits.
> 
> Add an error_exit function to print a helpful error message and a hint
> to run 'qemu-img --help' for more information.
> 
> As a bonus, "qemu-img <cmd> --help" now has a more reasonable exit code
> 0.
> 
> In the future the help text should be split by sub command, and only
> print the information for the specified command.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

This breaks 'qemu-img --help':

./qemu-img --help
qemu-img: Command not found: --help
Try 'qemu-img --help' for more information

See below:


<snip>
  
>      bs = bdrv_new_open("image", filename, fmt,
> @@ -2781,8 +2799,9 @@ int main(int argc, char **argv)
>  
>      qemu_init_main_loop();
>      bdrv_init();
> -    if (argc < 2)
> -        help();
> +    if (argc < 2) {
> +        error_exit("Not enough arguments");
> +    }

As an aside, are we sure we want './qemu-img' with no arguments to not
return the full help message?

>      cmdname = argv[1];
>      argc--; argv++;
>  
> @@ -2794,6 +2813,5 @@ int main(int argc, char **argv)
>      }
>  
>      /* not found */
> -    help();
> -    return 0;
> +    error_exit("Command not found: %s", cmdname);

Looks like we just relied previously on the default 'not found' case
for help() to provide the "--help" option.

>  }
> -- 
> 1.8.3.1
> 
>
Markus Armbruster April 25, 2014, 8:21 p.m. UTC | #2 
Jeff Cody <jcody@redhat.com> writes:

> On Wed, Apr 23, 2014 at 12:04:49PM +0200, Kevin Wolf wrote:
>> From: Fam Zheng <famz@redhat.com>
>> 
>> Previously, when there is a user error in argv parsing, qemu-img prints
>> help text and exits.
>> 
>> Add an error_exit function to print a helpful error message and a hint
>> to run 'qemu-img --help' for more information.
>> 
>> As a bonus, "qemu-img <cmd> --help" now has a more reasonable exit code
>> 0.
>> 
>> In the future the help text should be split by sub command, and only
>> print the information for the specified command.
>> 
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>
> This breaks 'qemu-img --help':
>
> ./qemu-img --help
> qemu-img: Command not found: --help
> Try 'qemu-img --help' for more information
>
> See below:
>
>
> <snip>
>   
>>      bs = bdrv_new_open("image", filename, fmt,
>> @@ -2781,8 +2799,9 @@ int main(int argc, char **argv)
>>  
>>      qemu_init_main_loop();
>>      bdrv_init();
>> -    if (argc < 2)
>> -        help();
>> +    if (argc < 2) {
>> +        error_exit("Not enough arguments");
>> +    }
>
> As an aside, are we sure we want './qemu-img' with no arguments to not
> return the full help message?

I hate it when programs spew out screenfulls of help on incorrect usage.
Three lines are the limit for me: error message, brief help, how to get
full help.

[...]
Fam Zheng April 28, 2014, 1:55 a.m. UTC | #3 
On Fri, 04/25 14:18, Jeff Cody wrote:
> On Wed, Apr 23, 2014 at 12:04:49PM +0200, Kevin Wolf wrote:
> > From: Fam Zheng <famz@redhat.com>
> > 
> > Previously, when there is a user error in argv parsing, qemu-img prints
> > help text and exits.
> > 
> > Add an error_exit function to print a helpful error message and a hint
> > to run 'qemu-img --help' for more information.
> > 
> > As a bonus, "qemu-img <cmd> --help" now has a more reasonable exit code
> > 0.
> > 
> > In the future the help text should be split by sub command, and only
> > print the information for the specified command.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> This breaks 'qemu-img --help':
> 
> ./qemu-img --help
> qemu-img: Command not found: --help
> Try 'qemu-img --help' for more information
> 
> See below:
> 
> 
> <snip>
>   
> >      bs = bdrv_new_open("image", filename, fmt,
> > @@ -2781,8 +2799,9 @@ int main(int argc, char **argv)
> >  
> >      qemu_init_main_loop();
> >      bdrv_init();
> > -    if (argc < 2)
> > -        help();
> > +    if (argc < 2) {
> > +        error_exit("Not enough arguments");
> > +    }
> 
> As an aside, are we sure we want './qemu-img' with no arguments to not
> return the full help message?
> 
> >      cmdname = argv[1];
> >      argc--; argv++;
> >  
> > @@ -2794,6 +2813,5 @@ int main(int argc, char **argv)
> >      }
> >  
> >      /* not found */
> > -    help();
> > -    return 0;
> > +    error_exit("Command not found: %s", cmdname);
> 
> Looks like we just relied previously on the default 'not found' case
> for help() to provide the "--help" option.
> 

Oops! Thanks for noticing this and sending the fix.

Fam
Mike D. Day April 30, 2014, 2:12 p.m. UTC | #4 
On Sun, Apr 27, 2014 at 9:55 PM, Fam Zheng <famz@redhat.com> wrote:
>> >      /* not found */
>> > -    help();
>> > -    return 0;
>> > +    error_exit("Command not found: %s", cmdname);
>>
>> Looks like we just relied previously on the default 'not found' case
>> for help() to provide the "--help" option.
>>
>
> Oops! Thanks for noticing this and sending the fix.

One of you guys going to send in the trivial patch to restore help?
(Or is it upstream in another patch?)

thanks, Mike
Eric Blake April 30, 2014, 2:16 p.m. UTC | #5 
On 04/30/2014 08:12 AM, Mike Day wrote:
> On Sun, Apr 27, 2014 at 9:55 PM, Fam Zheng <famz@redhat.com> wrote:
>>>>      /* not found */
>>>> -    help();
>>>> -    return 0;
>>>> +    error_exit("Command not found: %s", cmdname);
>>>
>>> Looks like we just relied previously on the default 'not found' case
>>> for help() to provide the "--help" option.
>>>
>>
>> Oops! Thanks for noticing this and sending the fix.
> 
> One of you guys going to send in the trivial patch to restore help?
> (Or is it upstream in another patch?)

Additional patches:
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg04074.html
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg04468.html
both already on the queue for the next PULL request of the block branch.
Mike D. Day April 30, 2014, 2:26 p.m. UTC | #6 
On Wed, Apr 30, 2014 at 10:16 AM, Eric Blake <eblake@redhat.com> wrote:
> Additional patches:
> https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg04074.html
> https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg04468.html
> both already on the queue for the next PULL request of the block branch.

Excellent, thank you!

Mike
diff mbox

Patch

diff --git a/qemu-img.c b/qemu-img.c
index fb626ac..4dae84a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -57,8 +57,22 @@  static void format_print(void *opaque, const char *name)
     printf(" %s", name);
 }
 
+static void QEMU_NORETURN GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...)
+{
+    va_list ap;
+
+    error_printf("qemu-img: ");
+
+    va_start(ap, fmt);
+    error_vprintf(fmt, ap);
+    va_end(ap);
+
+    error_printf("\nTry 'qemu-img --help' for more information\n");
+    exit(EXIT_FAILURE);
+}
+
 /* Please keep in synch with qemu-img.texi */
-static void help(void)
+static void QEMU_NORETURN help(void)
 {
     const char *help_msg =
            "qemu-img version " QEMU_VERSION ", Copyright (c) 2004-2008 Fabrice Bellard\n"
@@ -129,7 +143,7 @@  static void help(void)
     printf("%s\nSupported formats:", help_msg);
     bdrv_iterate_format(format_print, NULL);
     printf("\n");
-    exit(1);
+    exit(EXIT_SUCCESS);
 }
 
 static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)
@@ -399,7 +413,7 @@  static int img_create(int argc, char **argv)
     }
 
     if (optind >= argc) {
-        help();
+        error_exit("Expecting image file name");
     }
     optind++;
 
@@ -422,7 +436,7 @@  static int img_create(int argc, char **argv)
         img_size = (uint64_t)sval;
     }
     if (optind != argc) {
-        help();
+        error_exit("Unexpected argument: %s", argv[optind]);
     }
 
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
@@ -591,7 +605,8 @@  static int img_check(int argc, char **argv)
             } else if (!strcmp(optarg, "all")) {
                 fix = BDRV_FIX_LEAKS | BDRV_FIX_ERRORS;
             } else {
-                help();
+                error_exit("Unknown option value for -r "
+                           "(expecting 'leaks' or 'all'): %s", optarg);
             }
             break;
         case OPTION_OUTPUT:
@@ -603,7 +618,7 @@  static int img_check(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -714,7 +729,7 @@  static int img_commit(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -960,7 +975,7 @@  static int img_compare(int argc, char **argv)
 
 
     if (optind != argc - 2) {
-        help();
+        error_exit("Expecting two image file names");
     }
     filename1 = argv[optind++];
     filename2 = argv[optind++];
@@ -1276,7 +1291,7 @@  static int img_convert(int argc, char **argv)
     }
 
     if (bs_n < 1) {
-        help();
+        error_exit("Must specify image file name");
     }
 
 
@@ -1886,7 +1901,7 @@  static int img_info(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -2050,10 +2065,10 @@  static int img_map(int argc, char **argv)
             break;
         }
     }
-    if (optind >= argc) {
-        help();
+    if (optind != argc - 1) {
+        error_exit("Expecting one image file name");
     }
-    filename = argv[optind++];
+    filename = argv[optind];
 
     if (output && !strcmp(output, "json")) {
         output_format = OFORMAT_JSON;
@@ -2142,7 +2157,7 @@  static int img_snapshot(int argc, char **argv)
             return 0;
         case 'l':
             if (action) {
-                help();
+                error_exit("Cannot mix '-l', '-a', '-c', '-d'");
                 return 0;
             }
             action = SNAPSHOT_LIST;
@@ -2150,7 +2165,7 @@  static int img_snapshot(int argc, char **argv)
             break;
         case 'a':
             if (action) {
-                help();
+                error_exit("Cannot mix '-l', '-a', '-c', '-d'");
                 return 0;
             }
             action = SNAPSHOT_APPLY;
@@ -2158,7 +2173,7 @@  static int img_snapshot(int argc, char **argv)
             break;
         case 'c':
             if (action) {
-                help();
+                error_exit("Cannot mix '-l', '-a', '-c', '-d'");
                 return 0;
             }
             action = SNAPSHOT_CREATE;
@@ -2166,7 +2181,7 @@  static int img_snapshot(int argc, char **argv)
             break;
         case 'd':
             if (action) {
-                help();
+                error_exit("Cannot mix '-l', '-a', '-c', '-d'");
                 return 0;
             }
             action = SNAPSHOT_DELETE;
@@ -2179,7 +2194,7 @@  static int img_snapshot(int argc, char **argv)
     }
 
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -2292,8 +2307,11 @@  static int img_rebase(int argc, char **argv)
         progress = 0;
     }
 
-    if ((optind != argc - 1) || (!unsafe && !out_baseimg)) {
-        help();
+    if (optind != argc - 1) {
+        error_exit("Expecting one image file name");
+    }
+    if (!unsafe && !out_baseimg) {
+        error_exit("Must specify backing file (-b) or use unsafe mode (-u)");
     }
     filename = argv[optind++];
 
@@ -2553,7 +2571,7 @@  static int img_resize(int argc, char **argv)
     /* Remove size from argv manually so that negative numbers are not treated
      * as options by getopt. */
     if (argc < 3) {
-        help();
+        error_exit("Not enough arguments");
         return 1;
     }
 
@@ -2580,7 +2598,7 @@  static int img_resize(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -2697,7 +2715,7 @@  static int img_amend(int argc, char **argv)
     }
 
     if (!options) {
-        help();
+        error_exit("Must specify options (-o)");
     }
 
     filename = (optind == argc - 1) ? argv[argc - 1] : NULL;
@@ -2709,7 +2727,7 @@  static int img_amend(int argc, char **argv)
     }
 
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
 
     bs = bdrv_new_open("image", filename, fmt,
@@ -2781,8 +2799,9 @@  int main(int argc, char **argv)
 
     qemu_init_main_loop();
     bdrv_init();
-    if (argc < 2)
-        help();
+    if (argc < 2) {
+        error_exit("Not enough arguments");
+    }
     cmdname = argv[1];
     argc--; argv++;
 
@@ -2794,6 +2813,5 @@  int main(int argc, char **argv)
     }
 
     /* not found */
-    help();
-    return 0;
+    error_exit("Command not found: %s", cmdname);
 }