Re: [libvirt] [PATCH v2] qemu_migration: Avoid crashing if domain dies too quickly
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@redhat.com]
Sent: Friday, October 18, 2013 1:12 PM
To: Wangyufei (A)
Cc: libvir-list(a)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(a)redhat.com
>> [mailto:libvir-list-bounces@redhat.com] On Behalf Of Michal Privoznik
>> Sent: Friday, October 11, 2013 8:15 PM
>> To: libvir-list(a)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