Re: [Qemu-devel] [PATCH 28/47] qmp: add drive-mirror command

[Kevin Wolf <kwolf@redhat.com>]

Am 31.07.2012 11:33, schrieb Paolo Bonzini:
> Il 31/07/2012 11:26, Kevin Wolf ha scritto:
>> Am 24.07.2012 13:04, schrieb Paolo Bonzini:
>>> This adds the monitor commands that start the mirroring job.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> [ Moving the discussion upstream ]
>>
>>> Why make all of it inaccessible?  Everything except target device access
>>> does have a stable API.  The target device access can be delayed to 1.3,
>>> together with the much-needed QMP schema introspection.
>>
>> I'm not even sure about the QMP mirror command itself.
>>
>> I don't really like it, it does too many things at once: It can create
>> the target image file, it opens the target and it actually starts the
>> mirroring. It's rather bad at the first two steps, because it doesn't
>> take any options. This means that it can't create qcow2v3 images, for
>> example. Or you can't mirror into a backup with cache=unsafe while
>> running your real VM on cache=writethrough.
> 
> Yes, though this can be worked around with mode: 'existing'.

True. Only the problem with image creation, though, not the one with
bdrv_open() flags, right?

>> Having an all-in-one mirror command is a nice feature for HMP, but for
>> QMP it's more like a design problem.
>>
>> Now I see you have called it drive-mirror
> 
> I thought this was your idea. :)

Hm, then probably we discussed similar things before. :-)

>> , so that kind of implies that
>> it's not the final blockdev-mirror but just a QMP version of a command
>> primarily designed for HMP. As such this restricted functionality may be
>> acceptable, but it's not like everything is already perfect and there's
>> no room for discussion.
> 
> We keep going back to the same point that we do not have -blockdev, but
> it's becoming a bit frustrating to always rehash this same point...

The question is whether we need it at all. We do have a drive_add
if=none, and for creating a mirror target that should actually be enough.

Kevin