diff mbox

qemu-img: Improve error messages

Message ID 1398061384-2323-1-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng April 21, 2014, 6:23 a.m. UTC 
Previously, when there is an 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>
---
 qemu-img.c | 71 +++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 26 deletions(-)

Comments

Eric Blake April 21, 2014, 3:21 p.m. UTC | #1 
On 04/21/2014 12:23 AM, Fam Zheng wrote:
> Previously, when there is an user error in argv parsing, qemu-img prints

s/an user/a user/

(The rule of thumb for selecting which article to use for a leading 'u'
is pronunciation: anything starting with "you" uses "a", anything with
"uh" uses "an" [e.g. a unicorn under an umbrella].  Similar confusion
for 'h': hard h uses "a", silent h uses "an" [e.g. an hour and a half])

> 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>
> ---
>  qemu-img.c | 71 +++++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 45 insertions(+), 26 deletions(-)

>  
> +static void GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...)

Should you also mark this QEMU_NORETURN?

> +{
> +    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(1);

Worth using EXIT_FAILURE?

> @@ -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(0);

Worth using EXIT_SUCCESS?

> @@ -2046,10 +2061,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];

I had to look at context to see that the increment of optind was indeed
dead code worth removing.

> @@ -2788,6 +2807,6 @@ int main(int argc, char **argv)
>      }
>  
>      /* not found */
> -    help();
> +    error_exit("Command not found: %s", cmdname);
>      return 0;

This return is now dead code; using QEMU_NORETURN would have helped the
compiler flag it.
Fam Zheng April 22, 2014, 2 a.m. UTC | #2 
On Mon, 04/21 09:21, Eric Blake wrote:
> On 04/21/2014 12:23 AM, Fam Zheng wrote:
> > Previously, when there is an user error in argv parsing, qemu-img prints
> 
> s/an user/a user/
> 
> (The rule of thumb for selecting which article to use for a leading 'u'
> is pronunciation: anything starting with "you" uses "a", anything with
> "uh" uses "an" [e.g. a unicorn under an umbrella].  Similar confusion
> for 'h': hard h uses "a", silent h uses "an" [e.g. an hour and a half])

Yes, I typed as "an error^Wuser error" where it should be "an error^W^Wa user
error". Thanks for the explanation on article selection all the same!

> 
> > 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>
> > ---
> >  qemu-img.c | 71 +++++++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 45 insertions(+), 26 deletions(-)
> 
> >  
> > +static void GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...)
> 
> Should you also mark this QEMU_NORETURN?

Will do, and for help() as well.

> 
> > +{
> > +    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(1);
> 
> Worth using EXIT_FAILURE?

Yes.

> 
> > @@ -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(0);
> 
> Worth using EXIT_SUCCESS?

Yes.

Thanks,
Fam

> 
> > @@ -2046,10 +2061,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];
> 
> I had to look at context to see that the increment of optind was indeed
> dead code worth removing.
> 
> > @@ -2788,6 +2807,6 @@ int main(int argc, char **argv)
> >      }
> >  
> >      /* not found */
> > -    help();
> > +    error_exit("Command not found: %s", cmdname);
> >      return 0;
> 
> This return is now dead code; using QEMU_NORETURN would have helped the
> compiler flag it.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
diff mbox

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 8455994..d09655a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -57,6 +57,20 @@  static void format_print(void *opaque, const char *name)
     printf(" %s", name);
 }
 
+static void 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(1);
+}
+
 /* Please keep in synch with qemu-img.texi */
 static void help(void)
 {
@@ -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(0);
 }
 
 static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)
@@ -398,7 +412,7 @@  static int img_create(int argc, char **argv)
     }
 
     if (optind >= argc) {
-        help();
+        error_exit("Expecting image file name");
     }
     optind++;
 
@@ -421,7 +435,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,
@@ -590,7 +604,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:
@@ -602,7 +617,7 @@  static int img_check(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -713,7 +728,7 @@  static int img_commit(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -959,7 +974,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++];
@@ -1275,7 +1290,7 @@  static int img_convert(int argc, char **argv)
     }
 
     if (bs_n < 1) {
-        help();
+        error_exit("Must specify image file name");
     }
 
 
@@ -1882,7 +1897,7 @@  static int img_info(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -2046,10 +2061,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;
@@ -2138,7 +2153,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;
@@ -2146,7 +2161,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;
@@ -2154,7 +2169,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;
@@ -2162,7 +2177,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;
@@ -2175,7 +2190,7 @@  static int img_snapshot(int argc, char **argv)
     }
 
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -2288,8 +2303,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++];
 
@@ -2549,7 +2567,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;
     }
 
@@ -2576,7 +2594,7 @@  static int img_resize(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        help();
+        error_exit("Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -2692,7 +2710,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;
@@ -2704,7 +2722,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(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet);
@@ -2775,8 +2793,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++;
 
@@ -2788,6 +2807,6 @@  int main(int argc, char **argv)
     }
 
     /* not found */
-    help();
+    error_exit("Command not found: %s", cmdname);
     return 0;
 }