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

[Max Reitz]

On 12.04.2017 18:44, 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       | 84 
> +++++++++++++++++++++++++++++++++++++++-----------------
>  qemu-img.texi    | 12 ++++++--
>  3 files changed, 71 insertions(+), 29 deletions(-)

Looks good apart from some minor issues.

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index 83aff5e..31c4923 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -2090,6 +2100,11 @@ 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;

ret should be set to -1.

> +    }
> +
>      /* Initialize before goto out */
>      if (quiet) {
>          progress = 0;
> @@ -2100,8 +2115,13 @@ 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");
> +            goto fail_getopt;

Same here.

> +        }
>      }
>  
>      if (bs_n < 1) {

[...]

> diff --git a/qemu-img.texi b/qemu-img.texi
> index c81db3e..63f597d 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -45,9 +45,17 @@ keys.
>  
>  @item --image-opts
>  
> -Indicates that the @var{filename} parameter is to be interpreted as a
> +Indicates that the source @var{filename} parameter is to be interpreted as a
>  full option string, not a plain filename. This parameter is mutually
> -exclusive with the @var{-f} and @var{-F} parameters.
> +exclusive with the @var{-f} parameter.
> +
> address@hidden --target-image-opts
> +
> +Indicates that the target @var{filename} parameter(s) are to be interpreted a

s/a$/as/

> +a full option string, not a plain filename. This parameter is mutually
> +exclusive with the @var{-O} parameters. It is currently required to also use
> +the @var{-n} parameter to skip image creation. This restriction may be 
> relaxed
> +in a future release.

By the way, since you're referring to -n, you could just call it
"@var{output_filename}" instead of "target @var{filename}", because
that's how convert's target filename is described in this man page.

Max

>  
>  @item fmt
>  is the disk image format. It is guessed automatically in most cases. See 
> below
>

signature.asc
Description: OpenPGP digital signature