Re: [libvirt] [PATCHv4 08/18] blockjob: react to active block copy
On 04/13/2012 05:35 AM, Jiri Denemark wrote:
On Mon, Apr 09, 2012 at 21:52:17 -0600, Eric Blake wrote:
> For now, disk migration via block copy job is not implemented. But
> when we do implement it, we have to deal with the fact that qemu does
> not provide an easy way to re-start a qemu process with mirroring
> still intact (it _might_ be possible by using qemu -S then an
> initial 'drive-mirror' with disk reuse before starting the domain,
> but that gets hairy). Even something like 'virDomainSave' becomes
> hairy, if you realize the implications that 'virDomainRestore' would
> be stuck with recreating the same mirror layout.
>
> @@ -4947,6 +4952,12 @@ static virDomainPtr
qemudDomainDefine(virConnectPtr conn, const char *xml) {
> goto cleanup;
> }
> def = NULL;
> + if (virDomainHasDiskMirror(vm)) {
> + qemuReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
> + _("domain has active block copy job"));
> + virDomainObjAssignDef(vm, NULL, false);
> + goto cleanup;
> + }
> vm->persistent = 1;
I think it would be better to do this check a bit earlier in the process to
avoid the need to undo virDomainObjAssignDef().
I see where you are coming from in the limited context shown in this
patch (and it even matches my initial thoughts when first trying to
write the patch), but look at the bigger picture:
if (!(vm = virDomainAssignDef(driver->caps,
&driver->domains,
def, false))) {
goto cleanup;
}
def = NULL;
+ if (virDomainHasDiskMirror(vm)) {
+ qemuReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
+ _("domain has active block copy job"));
+ virDomainObjAssignDef(vm, NULL, false);
+ goto cleanup;
+ }
vm->persistent = 1;
That is, we don't know what vm is, to check if it has a disk mirror,
until after we have already associated a potential persistent definition
to vm, since virDomainAssignDef has the effect of doing both a lookup
and assignment in one function. The only way to avoid assigning the
persistent definition would be to do a lookup to see if there is already
an existing vm that matches the name/uuid in the def, but that still
implies that you have to have def available, and it also implies a
change to domain_conf.c to add a new entry point that does a lookup
without the assignment, just so we can probe for active mirrors, then
follow it by another lookup with assignment.
Therefore, I decided that it was easier to keep the existing lookup with
assignment and undo the assignment than to add new entry points and
redundant lookup work.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)