On Fri, Apr 13, 2012 at 09:28:08 -0600, Eric Blake wrote:
> 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
Oh, right. Although I was looking at this change in the context of complete
qemudDomainDefine(), I somehow missed the fact that virDomainAssignDef() does
the lookup.
OK, then.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list