Re: [Qemu-block] [PATCH v2 3/6] qemu-img: add support for conv=nocreat, notrunc args to dd command

[Max Reitz]

On 03.02.2017 13:02, Daniel P. Berrange wrote:
> The -n arg to the convert command allows use of a pre-existing image,
> rather than creating a new image. This adds equivalent functionality
> to the dd command using the 'conv' arg. If 'conv=nocreat' is used,
> then it will assume the image already exists. The existing image
> will be truncated to match the required output size. 'conv=notrunc'
> cna be used to preserve the existing image size.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  qemu-img-cmds.hx |   4 +-
>  qemu-img.c       | 137 
> +++++++++++++++++++++++++++++++++++++++++--------------
>  qemu-img.texi    |  10 +++-
>  3 files changed, 115 insertions(+), 36 deletions(-)

Quite frankly I don't like this patch very much. It's not bad in itself,
but I don't like the idea of giving qemu-img dd new features until it's
an interface for qemu-img convert. Everything that we add now encourages
new users to use it and will make the conversion a bit more difficult.

As long as qemu-img convert gets a --target-image-opts, I don't think we
need all of this functionality in qemu-img dd.

Anyway, I won't block/NACK this patch, so resuming review.

> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index f054599..b2c5424 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -46,9 +46,9 @@ STEXI
>  ETEXI
>  
>  DEF("dd", img_dd,
> -    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] 
> [count=blocks] [skip=blocks] if=input of=output")
> +    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] 
> [count=blocks] [skip=blocks] [conv=nocreat,notrunc] if=input of=output")
>  STEXI
> address@hidden dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] 
> address@hidden address@hidden address@hidden address@hidden address@hidden
> address@hidden dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] 
> address@hidden address@hidden address@hidden [conv=nocreat,notrunc] 
> address@hidden address@hidden

I'd just write something like address@hidden or I don't know. There
are other conversion specifiers (or however it's called) that we may
want to support in the future, e.g. sparse or noerror.

>  ETEXI
>  
>  DEF("info", img_info,
> diff --git a/qemu-img.c b/qemu-img.c
> index 629f9e9..c9ab9e5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c


[...]

> @@ -3906,6 +3910,31 @@ static int img_dd_skip(const char *arg,
>      return 0;
>  }
>  
> +static int img_dd_conv(const char *arg,
> +                       struct DdIo *in, struct DdIo *out,
> +                       struct DdInfo *dd)
> +{
> +    char **flags, **tmp;
> +
> +    tmp = flags = g_strsplit(arg, ",", 0);

"flag_pointer", "cur_flag_pointer" or "flat_iterator" instead of "tmp"
might be more suitable names.

> +
> +    while (tmp && *tmp) {
> +        if (g_str_equal(*tmp, "noconv")) {
> +            dd->flags |= C_NOCREAT;
> +        } else if (g_str_equal(*tmp, "notrunc")) {
> +            dd->flags |= C_NOTRUNC;
> +        } else {
> +            error_report("invalid conv argument: '%s'", *tmp);
> +            g_strfreev(flags);
> +            return 1;
> +        }
> +        tmp++;
> +    }
> +
> +    g_strfreev(flags);
> +    return 0;
> +}
> +
>  static int img_dd(int argc, char **argv)
>  {
>      int ret = 0;

[...]

> @@ -3954,7 +3984,7 @@ static int img_dd(int argc, char **argv)
>          { 0, 0, 0, 0 }
>      };
>  
> -    while ((c = getopt_long(argc, argv, "hf:O:", long_options, NULL))) {
> +    while ((c = getopt_long(argc, argv, "hnf:O:", long_options, NULL))) {

Looks like a relic from v1.

>          if (c == EOF) {
>              break;
>          }

[...]

> diff --git a/qemu-img.texi b/qemu-img.texi
> index 174aae3..9f10562 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -326,7 +326,7 @@ skipped. This is useful for formats such as @code{rbd} if 
> the target
>  volume has already been created with site specific options that cannot
>  be supplied through qemu-img.
>  
> address@hidden dd [-f @var{fmt}] [-O @var{output_fmt}] address@hidden 
> address@hidden address@hidden address@hidden address@hidden
> address@hidden dd [-f @var{fmt}] [-O @var{output_fmt}] address@hidden 
> address@hidden address@hidden [conv=nocreat,notrunc] address@hidden 
> address@hidden
>  
>  Dd copies from @var{input} file to @var{output} file converting it from
>  @var{fmt} format to @var{output_fmt} format.
> @@ -337,6 +337,14 @@ dd will stop reading input after reading @var{blocks} 
> input blocks.
>  
>  The size syntax is similar to dd(1)'s size syntax.
>  
> +If the @code{conv=nocreat} option is specified, the target volume creation
> +will be skipped. Its length will be truncated to match data length, if it
> +is longer than the required data size. If the @code{conv=notrunc} option
> +is specified, no file size shrinking will be done. If the existing output
> +file is too small it will be enlarged to fit. These options are useful for
> +formats such as @code{rbd} if the target volume has already been created
> +with site specific options that cannot be supplied through qemu-img.
> +

Good enough for now, but a list/table would probably be more extensible
in the future.

So all in all a patch that does well what it's supposed to do -- I just
don't quite agree on whether that goal is right.

Max

>  @item info [-f @var{fmt}] address@hidden [--backing-chain] @var{filename}
>  
>  Give information about the disk image @var{filename}. Use it in
>

signature.asc
Description: OpenPGP digital signature