Re: [Qemu-devel] [PATCH v2] qemu-img: Improve error messages

[Wang Sen]

On Tue, Apr 22, 2014 at 10:04:21AM +0800, Fam Zheng wrote:
> 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 <address@hidden>
> 
> ---
> v2: Address Eric's comments on QEMU_NORETURN, article and
>     EXIT_{SUCCEESS,FAILURE}.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  qemu-img.c | 73 
> +++++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 8455994..06e92f9 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, ...)
> @@ -398,7 +412,7 @@ static int img_create(int argc, char **argv)
>      }
> 
>      if (optind >= argc) {

optind has no chance to be larger than argc, right?

> -        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);

It would be better to remove "return 0" because you have add the QEMU_NORETURN
to the function error_exit.

>      return 0;
>  }
> -- 
> 1.9.2
> 
>