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

[Fam Zheng]

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 <address@hidden>
> > ---
> >  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
>