[5/7] qemu-img create: Support multiple -o options

Multiple -o options has the same meaning as having a single option with
all settings in the order of their respective -o options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

On Wed, 02/19 16:12, 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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-img.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index a889c84..30273ad 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -384,21 +384,18 @@ static int img_create(int argc, char **argv)
>          case 'e':
>              error_report("option -e is deprecated, please use \'-o "
>                    "encryption\' instead!");
> -            return 1;
> +            goto fail;
>          case '6':
>              error_report("option -6 is deprecated, please use \'-o "
>                    "compat6\' instead!");
> -            return 1;
> +            goto fail;
>          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);

OK. So what's done in patch 1 is masked here. Why not squash them and avoid the
intermediate state?  And, isn't original options leaked here?

Fam

>              }
>              break;
>          case 'q':
> @@ -431,7 +428,7 @@ static int img_create(int argc, char **argv)
>                  error_report("kilobytes, megabytes, gigabytes, terabytes, "
>                               "petabytes and exabytes.");
>              }
> -            return 1;
> +            goto fail;
>          }
>          img_size = (uint64_t)sval;
>      }
> @@ -440,6 +437,7 @@ static int img_create(int argc, char **argv)
>      }
>  
>      if (options_help) {
> +        g_free(options);
>          return print_block_option_help(filename, fmt);
>      }
>  
> @@ -448,10 +446,15 @@ static int img_create(int argc, char **argv)
>      if (error_is_set(&local_err)) {
>          error_report("%s: %s", filename, error_get_pretty(local_err));
>          error_free(local_err);
> -        return 1;
> +        goto fail;
>      }
>  
> +    g_free(options);
>      return 0;
> +
> +fail:
> +    g_free(options);
> +    return 1;
>  }
>  
>  static void dump_json_image_check(ImageCheck *check, bool quiet)
> -- 
> 1.8.1.4
> 
>

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).