| Message ID | 1398132261-19975-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <famz@redhat.com> > > --- > v2: Address Eric's comments on QEMU_NORETURN, article and > EXIT_{SUCCEESS,FAILURE}. > > Signed-off-by: Fam Zheng <famz@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 > >
On Tue, 04/22 11:31, Wang Sen wrote: > 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 <famz@redhat.com> > > > > --- > > v2: Address Eric's comments on QEMU_NORETURN, article and > > EXIT_{SUCCEESS,FAILURE}. > > > > Signed-off-by: Fam Zheng <famz@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? Yes. So here it could be if (optind == argc). Left for another patch. > > > - 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. OK, will do. Thanks! Fam > > > return 0; > > } > > -- > > 1.9.2 > > > > >
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) { - 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; }