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 <f...@redhat.com>
>
> ---
> v2: Address Eric's comments on QEMU_NORETURN, article and
> EXIT_{SUCCEESS,FAILURE}.
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
> 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
>
>
