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 <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?
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
> >
> >
>
