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

[Max Reitz]

On 02.05.2017 16:47, Daniel P. Berrange wrote:
> The '--image-opts' flag 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: Fam Zheng <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>

Sure you want to keep this, considering that there are quite some
changes since v5?

> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  qemu-img-cmds.hx |  4 +--
>  qemu-img.c       | 77 
> +++++++++++++++++++++++++++++++++++++-------------------
>  qemu-img.texi    | 12 +++++++--
>  3 files changed, 63 insertions(+), 30 deletions(-)

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index d8fdcb1..94c8cea 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -1900,7 +1901,7 @@ static int img_convert(int argc, char **argv)
>      char *options = NULL;
>      Error *local_err = NULL;
>      bool writethrough, src_writethrough, quiet = false, image_opts = false,
> -         skip_create = false, progress = false;
> +        skip_create = false, progress = false, tgt_image_opts = false;

Not sure about the indentation here. (I personally don't like spanning
the declaration over multiple lines in the first place, but that's a
different topic.) Indenting consecutive lines by four spaces is
standard, but the indentation by five spaces had a reason.

I guess I'd personally rather keep the five-space indentation...

>      int64_t ret = -EINVAL;
>  
>      ImgConvertState s = (ImgConvertState) {

[...]

> @@ -2047,12 +2056,22 @@ 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");
> +        goto fail_getopt;
> +    }
> +
>      s.src_num = argc - optind - 1;
>      out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
>  
>      if (options && has_help_option(options)) {
> -        ret = print_block_option_help(out_filename, out_fmt);
> -        goto fail_getopt;
> +        if (out_fmt) {
> +            ret = print_block_option_help(out_filename, out_fmt);
> +            goto out;

Shouldn't this remain goto fail_getopt;?

> +        } else {
> +            error_report("Option help requires a format be specified");
> +            goto fail_getopt;
> +        }
>      }
>  
>      if (s.src_num < 1) {

[...]

Why did you remove the compress &&
!out_bs->drv->bdrv_co_pwritev_compressed check? I liked it. :-(

Max

signature.asc
Description: OpenPGP digital signature