[libvirt] [PATCH v2] qemu_migration: Avoid crashing if domain dies too quickly

[Wangyufei (A)]

I'm sorry. I didn't get what you mean.

In virQEMUCapsInitQMP

    if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL)) ||
        !(vm = virDomainObjNew(xmlopt)))
        goto cleanup;

    vm->pid = pid;   //Apparently vm is not NULL here.

    if (!(mon = qemuMonitorOpen(vm, &config, true, &callbacks, NULL))) {  //If qemuMonitorOpen returns NULL here, but not do mon->vm = virObjectRef(vm); in qemuMonitorOpenInternal.
        ret = 0;
        goto cleanup;  // We go to cleanup here.
    }

    virObjectLock(mon);

    if (virQEMUCapsInitQMPMonitor(qemuCaps, mon) < 0)
        goto cleanup;

    ret = 0;

cleanup:
    if (mon)
        virObjectUnlock(mon);
    qemuMonitorClose(mon);
    virCommandAbort(cmd);
    virCommandFree(cmd);
    VIR_FREE(monarg);
    VIR_FREE(monpath);
    virObjectUnref(vm);    //vm is not NULL here, and we'll do something about vm->refs, right?
    virObjectUnref(xmlopt);

> -----Original Message-----
> From: Michal Privoznik [mailto:mprivozn at redhat.com]
> Sent: Friday, October 18, 2013 1:12 PM
> To: Wangyufei (A)
> Cc: libvir-list at redhat.com; Wangrui (K)
> Subject: Re: [libvirt] [PATCH v2] qemu_migration: Avoid crashing if domain
> dies too quickly
> 
> On 18.10.2013 06:06, Wangyufei (A) wrote:
> > Thanks at first, this patch some kinda solve my problem until now. But I still
> have a doubt about this patch.
> >
> >> -----Original Message-----
> >> From: libvir-list-bounces at redhat.com
> >> [mailto:libvir-list-bounces at redhat.com] On Behalf Of Michal Privoznik
> >> Sent: Friday, October 11, 2013 8:15 PM
> >> To: libvir-list at redhat.com
> >> Subject: [libvirt] [PATCH v2] qemu_migration: Avoid crashing if domain
> dies
> >> too quickly
> 
> >> @@ -2673,6 +2677,8 @@ cleanup:
> >>      virCommandFree(cmd);
> >>      VIR_FREE(monarg);
> >>      VIR_FREE(monpath);
> >> +    virObjectUnref(vm);
> >
> > Is this virObjectUnref(vm) corresponding to mon->vm = virObjectRef(vm)
> added in qemuMonitorOpenInternal?
> > If it is, how can we confirm virObjectRef(vm) has been done in
> qemuMonitorOpenInternal? If an error (anyone follows)happened in
> qemuMonitorOpenInternal is before mon->vm = virObjectRef(vm),
> > then we still goto cleanup and do virObjectUnref(vm), the refs will be
> wrong. Am I right?
> >
> 
> Unfortunately, you've cut off the chunk above that allocates @mon.
> Anyway, on initialization, @mon is filled with zeros. So until somebody
> sets mon->vm [1] mon->vm is effectively NULL. And virObjectUnref() acts
> like NOP on NULL.
> 
> >     if (!cb->eofNotify) {
> >         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >                        _("EOF notify callback must be supplied"));
> >         return NULL;
> >     }
> >     if (!cb->errorNotify) {
> >         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >                        _("Error notify callback must be supplied"));
> >         return NULL;
> >     }
> >
> >     if (qemuMonitorInitialize() < 0)
> >         return NULL;
> >
> >     if (!(mon = virObjectLockableNew(qemuMonitorClass)))
> >         return NULL;
> >
> >     mon->fd = -1;
> >     mon->logfd = -1;
> >     if (virCondInit(&mon->notify) < 0) {
> >         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >                        _("cannot initialize monitor condition"));
> >         goto cleanup;
> >     }
> >     mon->fd = fd;
> >     mon->hasSendFD = hasSendFD;
> >     mon->vm = virObjectRef(vm);
> 
> 1: ^^ until after this line
> >
> >> +    virObjectUnref(xmlopt);
> >>
> >>      if (pid != 0) {
> >>          char ebuf[1024];
> 
> I hope it makes things a bit clearer.
> 
> Michal