Re: [Qemu-block] [PATCH v2 5/6] qemu-img: introduce --target-image-opts for 'convert' command

[Max Reitz]

On 03.02.2017 13:02, 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.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  qemu-img-cmds.hx |   6 +--
>  qemu-img.c       | 131 
> ++++++++++++++++++++++++++++++++++++-------------------
>  qemu-img.texi    |  12 ++++-
>  3 files changed, 98 insertions(+), 51 deletions(-)

Apart from what the commit message says, it also introduces that switch
for dd, which I again don't like too much (quelle surprise), if only
because it requires conv=nocreat and thus patch 5 to be useful.

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index 39fcf09..dc4c6eb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -1918,7 +1933,7 @@ static int img_convert(int argc, char **argv)
>      bs_n = argc - optind - 1;
>      out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
>  
> -    if (options && has_help_option(options)) {
> +    if (out_fmt && options && has_help_option(options)) {

"!out_fmt && options && has_help_option(options)" should probably be an
error.

>          ret = print_block_option_help(out_filename, out_fmt);
>          goto out;
>      }
> @@ -1987,22 +2002,22 @@ static int img_convert(int argc, char **argv)
>          goto out;
>      }
>  
> -    /* Find driver and parse its options */
> -    drv = bdrv_find_format(out_fmt);
> -    if (!drv) {
> -        error_report("Unknown file format '%s'", out_fmt);
> -        ret = -1;
> -        goto out;
> -    }
> +    if (!skip_create) {
> +        /* Find driver and parse its options */
> +        drv = bdrv_find_format(out_fmt);
> +        if (!drv) {
> +            error_report("Unknown file format '%s'", out_fmt);
> +            ret = -1;
> +            goto out;
> +        }
>  
> -    proto_drv = bdrv_find_protocol(out_filename, true, &local_err);
> -    if (!proto_drv) {
> -        error_report_err(local_err);
> -        ret = -1;
> -        goto out;
> -    }
> +        proto_drv = bdrv_find_protocol(out_filename, true, &local_err);
> +        if (!proto_drv) {
> +            error_report_err(local_err);
> +            ret = -1;
> +            goto out;
> +        }
>  
> -    if (!skip_create) {
>          if (!drv->create_opts) {
>              error_report("Format driver '%s' does not support image 
> creation",
>                           drv->format_name);

Compression may be used with -n. This involves the check whether
drv->bdrv_co_pwritev_compressed is NULL or not -- which is bad if drv is
still NULL:

$ ./qemu-img create -f qcow2 foo.qcow2 64M
Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=off
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img convert -c -O qcow2 -n null-co:// foo.qcow2
[1]    17179 segmentation fault (core dumped)  ./qemu-img convert -c -O
qcow2 -n null-co:// foo.qcow

Therefore, you should probably only do the check whether compression is
supported if drv is non-NULL; and if it is NULL, do the check again
after the target image has been opened and its driver is known.

[...]

> @@ -4064,13 +4090,22 @@ static int img_dd(int argc, char **argv)
>          arg = NULL;
>      }
>  
> +    if (tgt_image_opts && !(dd.flags & C_NOCREAT)) {
> +        error_report("--target-image-opts requires use of -n flag");

*conv=nocreat

> +        goto out;
> +    }
> +
> +    if (!out_fmt && !tgt_image_opts) {
> +        out_fmt = "raw";
> +    }
> +
>      if (!(dd.flags & C_IF && dd.flags & C_OF)) {
>          error_report("Must specify both input and output files");
>          ret = -1;
>          goto out;
>      }
>  
> -    if (optionstr && has_help_option(optionstr)) {
> +    if (out_fmt && optionstr && has_help_option(optionstr)) {

Same as in img_convert().

>          ret = print_block_option_help(out.filename, out_fmt);
>          goto out;
>      }

[...]

> @@ -4152,7 +4187,6 @@ static int img_dd(int argc, char **argv)
>  
>      if (!(dd.flags & C_NOCREAT)) {
>          qemu_opt_set_number(opts, BLOCK_OPT_SIZE, out_size, &error_abort);
> -
>          ret = bdrv_create(drv, out.filename, opts, &local_err);
>          if (ret < 0) {
>              error_reportf_err(local_err,

I'm not sure whether this hunk is necessary...

[...]

> diff --git a/qemu-img.texi b/qemu-img.texi
> index 01acfb8..bda3cc3 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.

-F is correct, that's the one for qemu-img compare, after all (which
takes two source filenames).

> +
> address@hidden --target-image-opts
> +
> +Indicates that the target @var{filename} parameter(s) are to be interpreted a
> +a full option string, not a plain filename. This parameter is mutually
> +exclusive with the @var{-F} or @var{-O} parameters. It is currently required

-F is unrelated to a target ("output") file.

Max

> +to also use the @var{-n} parameter to skip image creation. This restriction
> +will be relaxed in a future release.
>  
>  @item fmt
>  is the disk image format. It is guessed automatically in most cases. See 
> below
>

signature.asc
Description: OpenPGP digital signature