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 <f...@redhat.com>
> > ---
> > 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
>
