Re: [Qemu-devel] [PATCH v5 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Max Reitz]

On 24.04.2017 11:16, Daniel P. Berrange wrote:
> The '--image-opts' flags indicates whether the source filename
> includes options. The target filename has to remain in the
> plain filename format though, since it needs to be passed to
> bdrv_create().  When using --skip-create though, it would be
> possible to use image-opts syntax. This adds --target-image-opts
> to indicate that the target filename includes options. Currently
> this mandates use of the --skip-create flag too.
> 
> Reviewed-by: Eric Blake <address@hidden>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  qemu-img-cmds.hx |  4 +--
>  qemu-img.c       | 86 
> ++++++++++++++++++++++++++++++++++++++++----------------
>  qemu-img.texi    | 12 ++++++--
>  3 files changed, 73 insertions(+), 29 deletions(-)

I'm afraid this will need a rebase onto block-next now (because of
Peter's "qemu-img: simplify img_convert" patch).

Besides the obvious rebase conflicts, there are some less obvious things
that may/should thus be changed:

> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 8ac7822..93b50ef 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx

[...]

> @@ -2090,6 +2100,12 @@ static int img_convert(int argc, char **argv)
>          goto fail_getopt;
>      }
>  
> +    if (tgt_image_opts && !skip_create) {
> +        error_report("--target-image-opts requires use of -n flag");
> +        ret = -1;

Depending on whether you decide to put this below
bdrv_parse_cache_mode() or above, you might actually no longer have to
set ret anymore.

> +        goto fail_getopt;
> +    }
> +
>      /* Initialize before goto out */
>      if (quiet) {
>          progress = 0;
> @@ -2100,8 +2116,14 @@ static int img_convert(int argc, char **argv)
>      out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
>  
>      if (options && has_help_option(options)) {
> -        ret = print_block_option_help(out_filename, out_fmt);
> -        goto out;
> +        if (out_fmt) {
> +            ret = print_block_option_help(out_filename, out_fmt);
> +            goto out;
> +        } else {
> +            error_report("Option help requires a format be specified");
> +            ret = -1;

Since this is most likely above bdrv_parse_cache_mode() (and may thus
end up above the previous hunk?), you probably don't have to set ret
here either (Sorry...).

The changes from v4 look good, but the rebase will result in non-trivial
changes, so I giving an R-b would not be of any real use, I'm afraid...

Max

> +            goto fail_getopt;
> +        }
>      }
>  
>      if (bs_n < 1) {

signature.asc
Description: OpenPGP digital signature