Re: [PATCH v2 1/1] qemu-img: do not erase destination file in qemu-img dd command

[Denis V. Lunev]

On 9/30/23 22:31, Mike Maslenkin wrote:
> Add a check that destination file exists and do not call bdrv_create for
> this case.
>
> Currently `qemu-img dd` command destroys content of destination file.
> Effectively this means that parameters (geometry) of destination image
> file are changing. This can be undesirable behavior for user especially
> if format of destination image does not support resizing.
>
> Steps to reproduce:
>    1. Create empty disk image with some non default size.
>         `qemu-img  create -f qcow2 $DEST_IMG 3T`
>       Remember that `qemu-img info $DEST_IMG` returns:
>         virtual size: 3 TiB (3298534883328 bytes)
>         disk size: 240 KiB
>         cluster_size: 65536
>    2. Run `qemu-img dd -O qcow2 of=$DEST_IMG if=$SRC_IMG bs=1M count=100`
>    3. Check `qemu-img info $DEST_IMG` output:
>         virtual size: 100 MiB (104857600 bytes)
>         disk size: 112 MiB
>         cluster_size: 65536
>
> Parameters of $DEST_IMG were changed. Actually `qemu-img dd` has created
> a new disk based on current default geometry for particular format.
> For example for "parallels" format default BAT for 256GB disk is written
> to empty file prior writing disk image data.
>
> With this patch virtual disk metadata and geometry of a destination image
> are preserved. As another visible change of `qemu-img dd` behavior is that
> if destination image is less than source it can finish with error (similar
> to "dd" utility):
>    qemu-img: error while writing to output image file: Input/output error
>
> Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com>
> ---
>    diff from v1: removed additional fprintf call leaved in patch by accident
> ---
>   qemu-img.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index a48edb71015c..1a83c14212fb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -5150,13 +5150,15 @@ static int img_dd(int argc, char **argv)
>                               size - in.bsz * in.offset, &error_abort);
>       }
>   
> -    ret = bdrv_create(drv, out.filename, opts, &local_err);
> -    if (ret < 0) {
> -        error_reportf_err(local_err,
> -                          "%s: error while creating output image: ",
> -                          out.filename);
> -        ret = -1;
> -        goto out;
> +    if (!g_file_test(out.filename, G_FILE_TEST_EXISTS)) {
> +        ret = bdrv_create(drv, out.filename, opts, &local_err);
> +        if (ret < 0) {
> +            error_reportf_err(local_err,
> +                               "%s: error while creating output image: ",
> +                               out.filename);
> +            ret = -1;
> +            goto out;
> +        }
>       }
>   
>       /* TODO, we can't honour --image-opts for the target,
may be it would be worth to follow conventional
'dd' approach, i.e. add conv=nocreat option which
will do the trick?

Den