Re: [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On 02/19/2014 08:12 AM, Kevin Wolf wrote:
> Multiple -o options has the same meaning as having a single option with
> all settings in the order of their respective -o options.

Ah, this addresses (part of) the problem I raised about patch 1.  But it
feels a bit awkward to have the intermediate state.

>          case 'o':
>              if (is_help_option(optarg)) {
>                  options_help = true;
>              } else if (!options) {
> -                options = optarg;
> +                options = g_strdup(optarg);
>              } else {
> -                error_report("-o cannot be used multiple times. Please use a "
> -                             "single -o option with comma-separated settings "
> -                             "instead.");
> -                return 1;
> +                options = g_strdup_printf("%s,%s", options, optarg);

In addition to the memleak that Fam pointed out, I still think you have
the problem that you aren't detecting:

-o backing_file=/path/to/foo,help

as specifying options_help.  I think you instead need to collect ALL -o
options without special-casing options_help, then at the end, call
contains_help_option which looks for "help" or "?" among all the
comma-separated options.


>  
> +    g_free(options);
>      return 0;
> +
> +fail:
> +    g_free(options);
> +    return 1;
>  }

I'd also like if we started using EXIT_FAILURE instead of the magic
number 1 (but that's probably a separate cleanup).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]