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

[Denis V. Lunev]

Can you please not top-post. This makes the discussion complex. This
approach is followed in this mailing list and in other similar lists
like LKML.

On 10/1/23 19:08, Mike Maslenkin wrote:
> I thought about "conv=notrunc", but my main concern is changed virtual
> disk metadata.
> It depends on how qemu-img used.
> May be I followed to wrong pattern, but pros and cons of adding "conv"
> parameter was not in my mind in scope of the first patch version.
> I see 4 obvious ways of using `qemu-img dd`:
> 1. Copy virtual disk data between images of same format. I think disk
> geometry must be preserved in this case.
> 2. Copy virtual disk data between different formats. It is a valid
> pattern? May be `qemu-img convert` should to be used instead?
> 3. Merge snapshots to specified disk image, i.e read current state and
> write it to new disk image.
> 4. Copy virtual disk data to raw binary file. Actually this patch
> breaks 'dd' behavior for this case when source image is less (in terms
> of logical blocks) than existed raw binary file.
>      May be for this case condition can be improved to smth like
>     if (strcmp(fmt, "raw") || !g_file_test(out.filename,
> G_FILE_TEST_EXISTS)) . And parameter "conv=notrunc" may be implemented
> additionally for this case.
My personal opinion is that qemu dd when you will need to
extract the SOME data from the original image and process
it further. Thus I use it to copy some data into raw binary
file. My next goal here would add ability to put data into
stdout that would be beneficial for. Though this is out of the
equation at the moment.

Though, speaking about the approach, I would say that the
patch changes current behavior which is not totally buggy
under a matter of this or that taste. It should be noted that
we are here in Linux world, not in the Mac world where we
were in position to avoid options and selections.

Thus my opinion that original behavior is to be preserved
as somebody is relying on it. The option you are proposing
seems valuable to me also and thus the switch is to be added.
The switch is well-defined in the original 'dd' world thus
either conv= option would be good, either nocreat or notrunc.
For me 'nocreat' seems more natural.

Anyway, the last word here belongs to either Hanna or Kevin ;)

Den

> Three of above do not require  "conv=" parameter from my point of view.
>
> I would be glad to hear other opinions.
>
> Regards,
> Mike.
>
>
> On Sun, Oct 1, 2023 at 3:25 PM Denis V. Lunev <den@virtuozzo.com> wrote:
> > 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