Google
  Web www.spinics.net

Re: [PATCHv4 08/18] blockjob: react to active block copy

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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


[Virt Tools]     [Libvirt Users]     [Fedora Users]     [Fedora Legacy]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [Yosemite Photos]     [KDE Users]     [Fedora Tools]

Powered by Linux

Google
  Web www.spinics.net