Re: [Qemu-devel] [PATCH v2 3/6] qemu-img convert: Support multiple -o options

[Jeff Cody]

On Thu, Feb 20, 2014 at 03:57:20PM +0100, Kevin Wolf wrote:
> Instead of ignoring all option values but the last one, multiple -o
> options now have the same meaning as having a single option with all
> settings in the order of their respective -o options.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  qemu-img.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 770e4e6..ba6e82d 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1160,6 +1160,9 @@ static int img_convert(int argc, char **argv)
>      Error *local_err = NULL;
>      QemuOpts *sn_opts = NULL;
>  
> +    /* Initialize before goto out */
> +    qemu_progress_init(progress, 1.0);
> +
>      fmt = NULL;
>      out_fmt = "raw";
>      cache = "unsafe";
> @@ -1191,13 +1194,21 @@ static int img_convert(int argc, char **argv)
>          case 'e':
>              error_report("option -e is deprecated, please use \'-o "
>                    "encryption\' instead!");
> -            return 1;
> +            ret = -1;
> +            goto out;
>          case '6':
>              error_report("option -6 is deprecated, please use \'-o "
>                    "compat6\' instead!");
> -            return 1;
> +            ret = -1;
> +            goto out;
>          case 'o':
> -            options = optarg;
> +            if (!options) {
> +                options = g_strdup(optarg);
> +            } else {
> +                char *old_options = options;
> +                options = g_strdup_printf("%s,%s", options, optarg);
> +                g_free(old_options);
> +            }
>              break;
>          case 's':
>              snapshot_name = optarg;
> @@ -1208,7 +1219,8 @@ static int img_convert(int argc, char **argv)
>                  if (!sn_opts) {
>                      error_report("Failed in parsing snapshot param '%s'",
>                                   optarg);
> -                    return 1;
> +                    ret = -1;
> +                    goto out;
>                  }
>              } else {
>                  snapshot_name = optarg;
> @@ -1221,7 +1233,8 @@ static int img_convert(int argc, char **argv)
>              sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);
>              if (sval < 0 || *end) {
>                  error_report("Invalid minimum zero buffer size for sparse 
> output specified");
> -                return 1;
> +                ret = -1;
> +                goto out;
>              }

There is a 'return -1' that is missed, that I think needs to be 'goto
out;' for cleanup.  It is right after the call to
bdrv_parse_cache_flags() in the img_convert() function:

    ret = bdrv_parse_cache_flags(cache, &flags);
    if (ret < 0) {
        error_report("Invalid cache option: %s", cache);
        return -1;
    }

Looks like this has been this way for a while, though.  And this is an
instance (the only instance) where img_convert returns -1 instead of 0
or 1.  I'm not sure the implications if that changed, and became '1',
so we'd probably want to preserve the return value.

Since this doesn't have anything to do with this patch (although
leaking 'options' now, in addition to the other stuff leaked), I'll go
ahead and give my reviewed-by, and we can fix this in another
follow-up patch:

Reviewed-by: Jeff Cody <address@hidden>

>  
>              min_sparse = sval / BDRV_SECTOR_SIZE;
> @@ -1253,10 +1266,7 @@ static int img_convert(int argc, char **argv)
>  
>      out_filename = argv[argc - 1];
>  
> -    /* Initialize before goto out */
> -    qemu_progress_init(progress, 1.0);
> -
> -    if (options && is_help_option(options)) {
> +    if (options && has_help_option(options)) {
>          ret = print_block_option_help(out_filename, out_fmt);
>          goto out;
>      }
> @@ -1649,6 +1659,7 @@ out:
>      free_option_parameters(create_options);
>      free_option_parameters(param);
>      qemu_vfree(buf);
> +    g_free(options);
>      if (sn_opts) {
>          qemu_opts_del(sn_opts);
>      }
> -- 
> 1.8.1.4
> 
>