[libvirt] [BUG] qemu quited unexpectedly will cause libvirtd crashed
When I edit the domain's config file like this: ===================== <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev='sdb' bus='scsi'/> <address type='drive' controller='0' bus='0' unit='10'/> </disk> ===================== Note, the unit is wrong, but libvirt does not check it. When I start the vm with the wrong config file, libvirtd will be blocked because qemu quited unexpectedly. This bug does not happen every time, and it only happened once on my box. So I try to use gdb and add sleep() to trigger this bug. I have posted two patches to fix 2 bugs. But there is still another bug, and I have no good way to fix it. I add sleep() in qemuDomainObjExitMonitorWithDriver(): ============================== int qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; int refs; int debug = 0; refs = qemuMonitorUnref(priv->mon); if (refs > 0) qemuMonitorUnlock(priv->mon); /* Note: qemu may quited unexpectedly here, and the monitor will be freed. * If it happened, priv->mon will be null. */ if (debug) sleep(100); qemuDriverLock(driver); virDomainObjLock(obj); if (refs == 0) { priv->mon = NULL; } } ============================== Steps to reproduce this bug: 1. use gdb to attach libvirtd, and set a breakpoint in the function qemuConnectMonitor() 2. start a vm 3. let the libvirtd to run until qemuMonitorSetCapabilities() returns. 4. kill the qemu process 5. step into qemuDomainObjExitMonitorWithDriver(), and set debug to 1 Now, qemuDomainObjExitMonitorWithDriver() will sleep 100s to make sure qemuProcessHandleMonitorEOF() is done before qemuProcessHandleMonitorEOF() returns. priv->mon will be null after qemuDomainObjExitMonitorWithDriver() returns. So we must not use it. Unfortunately we still use it, and it will cause libvirtd crashed. My first fix is that qemuDomainObjExitMonitorWithDriver() returns -1, and the caller checks the return value, then do some cleanup and return error. Unfortunately we may use priv->mon when doing some cleanup. The only way to avoid it is that add some local variable and set it when qemu quited unexpectedly. Avoid to use priv->mon in cleanup codes Is there some simply way to fix this bug.
On 03/29/2011 04:17 AM, Wen Congyang wrote:
When I edit the domain's config file like this: ===================== <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev='sdb' bus='scsi'/> <address type='drive' controller='0' bus='0' unit='10'/> </disk> =====================
Note, the unit is wrong, but libvirt does not check it. When I start the vm with the wrong config file, libvirtd will be blocked because qemu quited unexpectedly. This bug does not happen every time, and it only happened once on my box.
So I try to use gdb and add sleep() to trigger this bug. I have posted two patches to fix 2 bugs. But there is still another bug, and I have no good way to fix it.
Did you find a way to work around this yet? The typical solution is to temporarily add another reference to an object that you intend to still use after regaining locks, so that even if the qemu process dies in the meantime, it does not free the last reference and therefore does not delete the object in its cleanup code.
Steps to reproduce this bug: 1. use gdb to attach libvirtd, and set a breakpoint in the function qemuConnectMonitor() 2. start a vm 3. let the libvirtd to run until qemuMonitorSetCapabilities() returns. 4. kill the qemu process 5. step into qemuDomainObjExitMonitorWithDriver(), and set debug to 1
Now, qemuDomainObjExitMonitorWithDriver() will sleep 100s to make sure qemuProcessHandleMonitorEOF() is done before qemuProcessHandleMonitorEOF() returns.
priv->mon will be null after qemuDomainObjExitMonitorWithDriver() returns. So we must not use it. Unfortunately we still use it, and it will cause libvirtd crashed.
Sounds like qemuConnectMonitor needs an extra reference around priv->mon for the duration of the connect attempt, so that qemuProcessHandleMonitorEOF will not free the monitor. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
At 03/31/2011 03:59 AM, Eric Blake Write:
On 03/29/2011 04:17 AM, Wen Congyang wrote:
When I edit the domain's config file like this: ===================== <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev='sdb' bus='scsi'/> <address type='drive' controller='0' bus='0' unit='10'/> </disk> =====================
Note, the unit is wrong, but libvirt does not check it. When I start the vm with the wrong config file, libvirtd will be blocked because qemu quited unexpectedly. This bug does not happen every time, and it only happened once on my box.
So I try to use gdb and add sleep() to trigger this bug. I have posted two patches to fix 2 bugs. But there is still another bug, and I have no good way to fix it.
Did you find a way to work around this yet? The typical solution is to temporarily add another reference to an object that you intend to still use after regaining locks, so that even if the qemu process dies in the meantime, it does not free the last reference and therefore does not delete the object in its cleanup code.
Steps to reproduce this bug: 1. use gdb to attach libvirtd, and set a breakpoint in the function qemuConnectMonitor() 2. start a vm 3. let the libvirtd to run until qemuMonitorSetCapabilities() returns. 4. kill the qemu process 5. step into qemuDomainObjExitMonitorWithDriver(), and set debug to 1
Now, qemuDomainObjExitMonitorWithDriver() will sleep 100s to make sure qemuProcessHandleMonitorEOF() is done before qemuProcessHandleMonitorEOF() returns.
priv->mon will be null after qemuDomainObjExitMonitorWithDriver() returns. So we must not use it. Unfortunately we still use it, and it will cause libvirtd crashed.
Sounds like qemuConnectMonitor needs an extra reference around priv->mon for the duration of the connect attempt, so that qemuProcessHandleMonitorEOF will not free the monitor.
No, qemuConnectMonitor() calls qemuDomainObjEnterMonitorWithDriver() to hold an extra reference around priv->mon, and release it in qemuDomainObjExitMonitorWithDriver(). But qemuDomainObjExitMonitorWithDriver() does not tell the caller whether priv->mon can be used. If we will call qemuDomainObjEnterMonitorWithDriver()/qemuDomainObjEnterMonitor(), I think we must check whether the domain is active. If we call them more than once, we must check it every time. And we should not do other things between checking whether the domain is active and calling them(If we do as this, the code can be maintained easily) But some codes does not respect this rule. Here is the list of the functions that may have the similar problem: 1. qemu_migration.c doNativeMigrate() qemuMigrationToFile() 2. qemu_process.c qemuProcessWaitForMonitor() qemuProcessDetectVcpuPIDs() qemuProcessInitPasswords() qemuProcessInitPCIAddresses() qemuProcessStartCPUs() qemuProcessStopCPUs() qemuProcessStart() 3. qemu_hotplug.c qemuDomainChangeEjectableMedia() qemuDomainAttachPciDiskDevice() qemuDomainAttachPciControllerDevice qemuDomainAttachSCSIDisk() qemuDomainAttachUsbMassstorageDevice() qemuDomainAttachNetDevice() qemuDomainAttachHostPciDevice() qemuDomainAttachHostUsbDevice() qemuDomainDetachPciDiskDevice() qemuDomainDetachDiskDevice() qemuDomainDetachPciControllerDevice() qemuDomainDetachNetDevice() qemuDomainDetachHostPciDevice() qemuDomainDetachHostUsbDevice() qemuDomainChangeGraphicsPasswords() 4. qemu_driver.c qemudDomainHotplugVcpus() qemudDomainDumpXML() qemuDomainSnapshotCreateActive() qemuDomainMonitorCommand() If everyont agrees this rule, I will fix them. Thanks. Wen Congyang
On 03/31/2011 12:42 AM, Wen Congyang wrote:
So I try to use gdb and add sleep() to trigger this bug. I have posted two patches to fix 2 bugs. But there is still another bug, and I have no good way to fix it.
Steps to reproduce this bug: 1. use gdb to attach libvirtd, and set a breakpoint in the function qemuConnectMonitor() 2. start a vm 3. let the libvirtd to run until qemuMonitorSetCapabilities() returns. 4. kill the qemu process 5. step into qemuDomainObjExitMonitorWithDriver(), and set debug to 1
Now, qemuDomainObjExitMonitorWithDriver() will sleep 100s to make sure qemuProcessHandleMonitorEOF() is done before qemuProcessHandleMonitorEOF() returns.
priv->mon will be null after qemuDomainObjExitMonitorWithDriver() returns. So we must not use it. Unfortunately we still use it, and it will cause libvirtd crashed.
Sounds like qemuConnectMonitor needs an extra reference around priv->mon for the duration of the connect attempt, so that qemuProcessHandleMonitorEOF will not free the monitor.
No, qemuConnectMonitor() calls qemuDomainObjEnterMonitorWithDriver() to hold an extra reference around priv->mon, and release it in qemuDomainObjExitMonitorWithDriver(). But qemuDomainObjExitMonitorWithDriver() does not tell the caller whether priv->mon can be used.
If we will call qemuDomainObjEnterMonitorWithDriver()/qemuDomainObjEnterMonitor(), I think we must check whether the domain is active. If we call them more than once, we must check it every time. And we should not do other things between checking whether the domain is active and calling them(If we do as this, the code can be maintained easily)
I think I hit on the same problem earlier, of a guest modifying vm state on assumption that a successful monitor command even if the vm went down in the window after the monitor command completed but before lock was regained: https://www.redhat.com/archives/libvir-list/2011-March/msg00636.html
But some codes does not respect this rule.
Here is the list of the functions that may have the similar problem: 1. qemu_migration.c doNativeMigrate() qemuMigrationToFile()
My current thoughts are that maybe qemuDomainObjExitMonitor should be made to return the value of vm active after regaining lock, and marked ATTRIBUTE_RETURN_CHECK, to force all other callers to detect the case of a monitor command completing successfully but then the VM disappearing in the window between command completion and regaining the vm lock. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
At 04/01/2011 02:54 AM, Eric Blake Write:
On 03/31/2011 12:42 AM, Wen Congyang wrote:
So I try to use gdb and add sleep() to trigger this bug. I have posted two patches to fix 2 bugs. But there is still another bug, and I have no good way to fix it.
Steps to reproduce this bug: 1. use gdb to attach libvirtd, and set a breakpoint in the function qemuConnectMonitor() 2. start a vm 3. let the libvirtd to run until qemuMonitorSetCapabilities() returns. 4. kill the qemu process 5. step into qemuDomainObjExitMonitorWithDriver(), and set debug to 1
Now, qemuDomainObjExitMonitorWithDriver() will sleep 100s to make sure qemuProcessHandleMonitorEOF() is done before qemuProcessHandleMonitorEOF() returns.
priv->mon will be null after qemuDomainObjExitMonitorWithDriver() returns. So we must not use it. Unfortunately we still use it, and it will cause libvirtd crashed.
Sounds like qemuConnectMonitor needs an extra reference around priv->mon for the duration of the connect attempt, so that qemuProcessHandleMonitorEOF will not free the monitor.
No, qemuConnectMonitor() calls qemuDomainObjEnterMonitorWithDriver() to hold an extra reference around priv->mon, and release it in qemuDomainObjExitMonitorWithDriver(). But qemuDomainObjExitMonitorWithDriver() does not tell the caller whether priv->mon can be used.
If we will call qemuDomainObjEnterMonitorWithDriver()/qemuDomainObjEnterMonitor(), I think we must check whether the domain is active. If we call them more than once, we must check it every time. And we should not do other things between checking whether the domain is active and calling them(If we do as this, the code can be maintained easily)
I think I hit on the same problem earlier, of a guest modifying vm state on assumption that a successful monitor command even if the vm went down in the window after the monitor command completed but before lock was regained:
https://www.redhat.com/archives/libvir-list/2011-March/msg00636.html
But some codes does not respect this rule.
Here is the list of the functions that may have the similar problem: 1. qemu_migration.c doNativeMigrate() qemuMigrationToFile()
My current thoughts are that maybe qemuDomainObjExitMonitor should be made to return the value of vm active after regaining lock, and marked ATTRIBUTE_RETURN_CHECK, to force all other callers to detect the case of a monitor command completing successfully but then the VM disappearing in the window between command completion and regaining the vm lock.
We can make qemuDomainObjExitMonitor() to return a value of vm active after regaining lock, and mark it ATTRIBUTE_RETURN_CHECK. The compiler can check the problem. But if we do something like this: { ... qemuDomainObjBeginJob(); qemuDomainObjEnterMonitor(); ... } Libvirtd still may crash as vm is inactive when qemuDomainObjBeginJob() returns and vm->priv->mon is NULL. function a() { ... qemuDomainObjEnterMonitor(); /* invoke monitor command */ qemuDomainObjExitMonitor(); ... } function b() { ... ignore_value(a()); ... qemuDomainObjEnterMonitor(); ... } If invoking monitor command failed is not a fatal problem, and we ignore the return value of function a(). libvirtd may crash if a() failed. The complier can not detect the above problems. If we always check whether vm is active before calling qemuDomainObjEnterMonitor(), we can avoid this problem.
qemu may quited unexpectedly when invoking a monitor command. And priv->mon will be NULL after qemuDomainObjExitMonitor* returns. So we must not use it. Unfortunately we still use it, and it will cause libvirtd crashed. As Eric suggested, qemuDomainObjExitMonitor* should be made to return the value of vm active after regaining lock, and marked ATTRIBUTE_RETURN_CHECK, to force all other callers to detect the case of a monitor command completing successfully but then the VM disappearing in the window between command completion and regaining the vm lock. --- src/qemu/qemu_domain.c | 24 ++++- src/qemu/qemu_domain.h | 6 +- src/qemu/qemu_driver.c | 87 +++++++++++++---- src/qemu/qemu_hotplug.c | 230 ++++++++++++++++++++++++++++----------------- src/qemu/qemu_migration.c | 93 +++++++++++++------ src/qemu/qemu_process.c | 61 +++++++++--- 6 files changed, 344 insertions(+), 157 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c2a1f9a..0d40b7e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -578,7 +578,7 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) * * Should be paired with an earlier qemuDomainObjEnterMonitor() call */ -void qemuDomainObjExitMonitor(virDomainObjPtr obj) +int qemuDomainObjExitMonitor(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; int refs; @@ -588,11 +588,20 @@ void qemuDomainObjExitMonitor(virDomainObjPtr obj) if (refs > 0) qemuMonitorUnlock(priv->mon); + /* Note: qemu may quited unexpectedly here, and the monitor will be freed. + * If it happened, priv->mon will be null. + */ + virDomainObjLock(obj); if (refs == 0) { priv->mon = NULL; } + + if (priv->mon == NULL) + return -1; + else + return 0; } @@ -621,8 +630,8 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, * * Should be paired with an earlier qemuDomainObjEnterMonitorWithDriver() call */ -void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, - virDomainObjPtr obj) +int qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, + virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; int refs; @@ -632,12 +641,21 @@ void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, if (refs > 0) qemuMonitorUnlock(priv->mon); + /* Note: qemu may quited unexpectedly here, and the monitor will be freed. + * If it happened, priv->mon will be null. + */ + qemuDriverLock(driver); virDomainObjLock(obj); if (refs == 0) { priv->mon = NULL; } + + if (priv->mon == NULL) + return -1; + else + return 0; } void qemuDomainObjEnterRemoteWithDriver(struct qemud_driver *driver, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8258900..92fccda 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -99,11 +99,11 @@ int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver, virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; int qemuDomainObjEndJob(virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; void qemuDomainObjEnterMonitor(virDomainObjPtr obj); -void qemuDomainObjExitMonitor(virDomainObjPtr obj); +int qemuDomainObjExitMonitor(virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, virDomainObjPtr obj); -void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, - virDomainObjPtr obj); +int qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, + virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; void qemuDomainObjEnterRemoteWithDriver(struct qemud_driver *driver, virDomainObjPtr obj); void qemuDomainObjExitRemoteWithDriver(struct qemud_driver *driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 04a5f65..2d41576 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1452,7 +1452,11 @@ static int qemudDomainShutdown(virDomainPtr dom) { priv = vm->privateData; qemuDomainObjEnterMonitor(vm); ret = qemuMonitorSystemPowerdown(priv->mon); - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } endjob: if (qemuDomainObjEndJob(vm) == 0) @@ -1659,7 +1663,11 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, priv = vm->privateData; qemuDomainObjEnterMonitor(vm); r = qemuMonitorSetBalloon(priv->mon, newmem); - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + r = -1; + } qemuAuditMemory(vm, vm->def->mem.cur_balloon, newmem, "update", r == 1); if (r < 0) @@ -1749,7 +1757,11 @@ static int qemudDomainGetInfo(virDomainPtr dom, else { qemuDomainObjEnterMonitor(vm); err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + err = -1; + } } if (qemuDomainObjEndJob(vm) == 0) { vm = NULL; @@ -2524,7 +2536,11 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus) ret = 0; cleanup: - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } vm->def->vcpus = vcpus; qemuAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1); return ret; @@ -3295,7 +3311,11 @@ static char *qemudDomainDumpXML(virDomainPtr dom, qemuDomainObjEnterMonitorWithDriver(driver, vm); err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + err = -1; + } if (qemuDomainObjEndJob(vm) == 0) { vm = NULL; goto cleanup; @@ -4843,7 +4863,11 @@ qemudDomainBlockStats (virDomainPtr dom, &stats->wr_req, &stats->wr_bytes, &stats->errs); - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } endjob: if (qemuDomainObjEndJob(vm) == 0) @@ -4944,7 +4968,11 @@ qemudDomainMemoryStats (virDomainPtr dom, qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitor(vm); ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats); - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } } else { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -5085,17 +5113,18 @@ qemudDomainMemoryPeek (virDomainPtr dom, priv = vm->privateData; qemuDomainObjEnterMonitor(vm); if (flags == VIR_MEMORY_VIRTUAL) { - if (qemuMonitorSaveVirtualMemory(priv->mon, offset, size, tmp) < 0) { - qemuDomainObjExitMonitor(vm); - goto endjob; - } + ret = qemuMonitorSaveVirtualMemory(priv->mon, offset, size, tmp); } else { - if (qemuMonitorSavePhysicalMemory(priv->mon, offset, size, tmp) < 0) { - qemuDomainObjExitMonitor(vm); - goto endjob; - } + ret = qemuMonitorSavePhysicalMemory(priv->mon, offset, size, tmp); } - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } + + if (ret < -1) + goto endjob; /* Read the memory file into buffer. */ if (saferead (fd, buffer, size) == (ssize_t) -1) { @@ -5259,7 +5288,11 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, ret = qemuMonitorGetBlockExtent(priv->mon, disk->info.alias, &info->allocation); - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } } if (qemuDomainObjEndJob(vm) == 0) @@ -6101,7 +6134,11 @@ qemuDomainSnapshotCreateActive(virConnectPtr conn, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } cleanup: if (resume && virDomainObjIsActive(vm) && @@ -6434,7 +6471,11 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + rc = -1; + } if (rc < 0) goto endjob; } @@ -6557,7 +6598,7 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); /* we continue on even in the face of error */ qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitorWithDriver(driver, vm); + ignore_value(qemuDomainObjExitMonitorWithDriver(driver, vm)); } if (snap == vm->current_snapshot) { @@ -6770,7 +6811,11 @@ static int qemuDomainMonitorCommand(virDomainPtr domain, const char *cmd, goto cleanup; qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorArbitraryCommand(priv->mon, cmd, result, hmp); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } if (qemuDomainObjEndJob(vm) == 0) { vm = NULL; goto cleanup; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b03f774..d70e426 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -106,7 +106,11 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver, } else { ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force); } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditDisk(vm, origdisk, disk, "update", ret >= 0); @@ -201,7 +205,11 @@ int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, memcpy(&disk->info.addr.pci, &guestAddr, sizeof(guestAddr)); } } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditDisk(vm, NULL, disk, "attach", ret >= 0); @@ -277,7 +285,11 @@ int qemuDomainAttachPciControllerDevice(struct qemud_driver *driver, type, &controller->info.addr.pci); } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } if (ret == 0) { controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; @@ -433,7 +445,11 @@ int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, memcpy(&disk->info.addr.drive, &driveAddr, sizeof(driveAddr)); } } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditDisk(vm, NULL, disk, "attach", ret >= 0); @@ -516,7 +532,11 @@ int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, } else { ret = qemuMonitorAddUSBDisk(priv->mon, disk->src); } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditDisk(vm, NULL, disk, "attach", ret >= 0); @@ -632,21 +652,22 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfd_name, - vhostfd, vhostfd_name) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuAuditNet(vm, NULL, net, "attach", false); - goto cleanup; - } + ret = qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfd_name, + vhostfd, vhostfd_name); } else { - if (qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfd_name, - vhostfd, vhostfd_name) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuAuditNet(vm, NULL, net, "attach", false); - goto cleanup; - } + ret = qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfd_name, + vhostfd, vhostfd_name); } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + + qemuAuditNet(vm, NULL, net, "attach", ret >= 0); + + if (ret < 0) + goto cleanup; VIR_FORCE_CLOSE(tapfd); VIR_FORCE_CLOSE(vhostfd); @@ -667,25 +688,26 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuAuditNet(vm, NULL, net, "attach", false); - goto try_remove; - } + ret = qemuMonitorAddDevice(priv->mon, nicstr); } else { - if (qemuMonitorAddPCINetwork(priv->mon, nicstr, - &guestAddr) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuAuditNet(vm, NULL, net, "attach", false); - goto try_remove; + ret = qemuMonitorAddPCINetwork(priv->mon, nicstr, + &guestAddr); + if (ret == 0) { + net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + memcpy(&net->info.addr.pci, &guestAddr, sizeof(guestAddr)); } - net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - memcpy(&net->info.addr.pci, &guestAddr, sizeof(guestAddr)); } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditNet(vm, NULL, net, "attach", true); + if (ret < 0) + goto try_remove; + ret = 0; vm->def->nets[vm->def->nnets++] = net; @@ -723,7 +745,9 @@ try_remove: if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) VIR_WARN("Failed to remove network backend for netdev %s", netdev_name); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); VIR_FREE(netdev_name); } else { VIR_WARN0("Unable to remove network backend"); @@ -736,7 +760,9 @@ try_remove: if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) VIR_WARN("Failed to remove network backend for vlan %d, net %s", vlan, hostnet_name); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); VIR_FREE(hostnet_name); } goto cleanup; @@ -795,7 +821,11 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, configfd, configfd_name); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto error; + } } else { virDomainDevicePCIAddress guestAddr; @@ -803,7 +833,11 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, ret = qemuMonitorAddPCIHostDevice(priv->mon, &hostdev->source.subsys.u.pci, &guestAddr); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto error; + } hostdev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; memcpy(&hostdev->info.addr.pci, &guestAddr, sizeof(guestAddr)); @@ -886,7 +920,11 @@ int qemuDomainAttachHostUsbDevice(struct qemud_driver *driver, ret = qemuMonitorAddUSBDeviceExact(priv->mon, hostdev->source.subsys.u.usb.bus, hostdev->source.subsys.u.usb.device); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditHostdev(vm, hostdev, "attach", ret == 0); if (ret < 0) goto error; @@ -1145,25 +1183,27 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - qemuDomainObjExitMonitor(vm); - goto cleanup; - } + ret = qemuMonitorDelDevice(priv->mon, detach->info.alias); } else { - if (qemuMonitorRemovePCIDevice(priv->mon, - &detach->info.addr.pci) < 0) { - qemuDomainObjExitMonitor(vm); - goto cleanup; - } + ret = qemuMonitorRemovePCIDevice(priv->mon, + &detach->info.addr.pci); } /* disconnect guest from host device */ - qemuMonitorDriveDel(priv->mon, drivestr); + if (ret == 0) + qemuMonitorDriveDel(priv->mon, drivestr); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditDisk(vm, detach, NULL, "detach", ret >= 0); + if (ret < 0) + goto cleanup; + if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE) && qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) VIR_WARN("Unable to release PCI address on %s", dev->data.disk->src); @@ -1235,18 +1275,23 @@ int qemuDomainDetachDiskDevice(struct qemud_driver *driver, } qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - qemuDomainObjExitMonitor(vm); - goto cleanup; - } + ret = qemuMonitorDelDevice(priv->mon, detach->info.alias); /* disconnect guest from host device */ - qemuMonitorDriveDel(priv->mon, drivestr); + if (ret == 0) + qemuMonitorDriveDel(priv->mon, drivestr); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditDisk(vm, detach, NULL, "detach", ret >= 0); + if (ret < 0) + goto cleanup; + virDomainDiskRemove(vm->def, i); virDomainDiskDefFree(detach); @@ -1364,18 +1409,19 @@ int qemuDomainDetachPciControllerDevice(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { - qemuDomainObjExitMonitor(vm); - goto cleanup; - } + ret = qemuMonitorDelDevice(priv->mon, detach->info.alias); } else { - if (qemuMonitorRemovePCIDevice(priv->mon, - &detach->info.addr.pci) < 0) { - qemuDomainObjExitMonitor(vm); - goto cleanup; - } + ret = qemuMonitorRemovePCIDevice(priv->mon, + &detach->info.addr.pci); + } + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; } - qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (ret < 0) + goto cleanup; if (vm->def->ncontrollers > 1) { memmove(vm->def->controllers + i, @@ -1452,35 +1498,31 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - qemuDomainObjExitMonitor(vm); - qemuAuditNet(vm, detach, NULL, "detach", false); - goto cleanup; - } + ret = qemuMonitorDelDevice(priv->mon, detach->info.alias); } else { - if (qemuMonitorRemovePCIDevice(priv->mon, - &detach->info.addr.pci) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuAuditNet(vm, detach, NULL, "detach", false); - goto cleanup; - } + ret = qemuMonitorRemovePCIDevice(priv->mon, + &detach->info.addr.pci); } + if (ret < 0) + goto exitmonitor; + if (qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuAuditNet(vm, detach, NULL, "detach", false); - goto cleanup; - } + ret = qemuMonitorRemoveNetdev(priv->mon, hostnet_name); } else { - if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuAuditNet(vm, detach, NULL, "detach", false); - goto cleanup; - } + ret = qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name); } - qemuDomainObjExitMonitorWithDriver(driver, vm); + +exitmonitor: + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } + + if (ret < 0) + goto cleanup; qemuAuditNet(vm, detach, NULL, "detach", true); @@ -1582,7 +1624,11 @@ int qemuDomainDetachHostPciDevice(struct qemud_driver *driver, } else { ret = qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci); } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditHostdev(vm, detach, "detach", ret == 0); if (ret < 0) return -1; @@ -1681,7 +1727,11 @@ int qemuDomainDetachHostUsbDevice(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info.alias); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } qemuAuditHostdev(vm, detach, "detach", ret == 0); if (ret < 0) return -1; @@ -1798,7 +1848,11 @@ qemuDomainChangeGraphicsPasswords(struct qemud_driver *driver, } cleanup: - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 43741e1..cc8f428 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -125,7 +125,11 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm) VIR_DEBUG0("Cancelling job at client request"); qemuDomainObjEnterMonitorWithDriver(driver, vm); rc = qemuMonitorMigrateCancel(priv->mon); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + rc = -1; + } if (rc < 0) { VIR_WARN0("Unable to cancel job"); } @@ -142,7 +146,11 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm) VIR_DEBUG("Setting migration downtime to %llums", ms); qemuDomainObjEnterMonitorWithDriver(driver, vm); rc = qemuMonitorSetMigrationDowntime(priv->mon, ms); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + rc = -1; + } if (rc < 0) VIR_WARN0("Unable to set migration downtime"); } else if (priv->jobSignals & QEMU_JOB_SIGNAL_MIGRATE_SPEED) { @@ -153,7 +161,11 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm) VIR_DEBUG("Setting migration bandwidth to %luMbs", bandwidth); qemuDomainObjEnterMonitorWithDriver(driver, vm); rc = qemuMonitorSetMigrationSpeed(priv->mon, bandwidth); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + rc = -1; + } if (rc < 0) VIR_WARN0("Unable to set migration speed"); } @@ -173,7 +185,11 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm) &memProcessed, &memRemaining, &memTotal); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + rc = -1; + } if (rc < 0) { priv->jobInfo.type = VIR_DOMAIN_JOB_FAILED; @@ -608,24 +624,31 @@ static int doNativeMigrate(struct qemud_driver *driver, } qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (resource > 0 && - qemuMonitorSetMigrationSpeed(priv->mon, resource) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; + if (resource > 0) { + ret = qemuMonitorSetMigrationSpeed(priv->mon, resource); } + if (ret < 0) + goto exitmonitor; + if (flags & VIR_MIGRATE_NON_SHARED_DISK) background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK; if (flags & VIR_MIGRATE_NON_SHARED_INC) background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC; - if (qemuMonitorMigrateToHost(priv->mon, background_flags, uribits->server, - uribits->port) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; + ret = qemuMonitorMigrateToHost(priv->mon, background_flags, uribits->server, + uribits->port); + +exitmonitor: + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; } - qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (ret < 0) + goto cleanup; if (qemuMigrationWaitForCompletion(driver, vm) < 0) goto cleanup; @@ -824,7 +847,11 @@ static int doTunnelMigrate(struct qemud_driver *driver, } else { internalret = -1; } - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + internalret = -1; + } if (internalret < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("tunnelled migration monitor command failed")); @@ -844,15 +871,16 @@ static int doTunnelMigrate(struct qemud_driver *driver, * rather failed later on. Check the output of "info migrate" */ qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorGetMigrationStatus(priv->mon, - &status, - &transferred, - &remaining, - &total) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cancel; + retval = qemuMonitorGetMigrationStatus(priv->mon, + &status, + &transferred, + &remaining, + &total); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + retval = -1; } - qemuDomainObjExitMonitorWithDriver(driver, vm); if (status == QEMU_MONITOR_MIGRATION_STATUS_ERROR) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -875,7 +903,10 @@ cancel: if (retval != 0 && virDomainObjIsActive(vm)) { qemuDomainObjEnterMonitorWithDriver(driver, vm); qemuMonitorMigrateCancel(priv->mon); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + } } finish: @@ -1372,12 +1403,12 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, if (virSetCloseExec(pipeFD[1]) < 0) { virReportSystemError(errno, "%s", _("Unable to set cloexec flag")); - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; + rc = -1; + goto exitmonitor; } if (virCommandRunAsync(cmd, NULL) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; + rc = -1; + goto exitmonitor; } rc = qemuMonitorMigrateToFd(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, @@ -1391,7 +1422,13 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, args, path, offset); } } - qemuDomainObjExitMonitorWithDriver(driver, vm); + +exitmonitor: + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + rc = -1; + } if (rc < 0) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9ada24d..4ca70bc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -662,7 +662,11 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm) qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorSetCapabilities(priv->mon); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } error: @@ -1070,7 +1074,11 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorGetPtyPaths(priv->mon, paths); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } VIR_DEBUG("qemuMonitorGetPtyPaths returned %i", ret); if (ret == 0) @@ -1122,11 +1130,15 @@ qemuProcessDetectVcpuPIDs(struct qemud_driver *driver, /* What follows is now all KVM specific */ qemuDomainObjEnterMonitorWithDriver(driver, vm); - if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - return -1; + ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ncpupids = -1; } - qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (ncpupids < 0) + return -1; /* Treat failure to get VCPU<->PID mapping as non-fatal */ if (ncpupids == 0) @@ -1322,7 +1334,11 @@ qemuProcessInitPasswords(virConnectPtr conn, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorSetDrivePassphrase(priv->mon, alias, secret); VIR_FREE(secret); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } if (ret < 0) goto cleanup; } @@ -1713,7 +1729,12 @@ qemuProcessInitPCIAddresses(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); naddrs = qemuMonitorGetAllPCIAddresses(priv->mon, &addrs); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + VIR_FREE(addrs); + return -1; + } ret = qemuProcessDetectPCIAddresses(vm, addrs, naddrs); @@ -1890,7 +1911,11 @@ qemuProcessStartCPUs(struct qemud_driver *driver, virDomainObjPtr vm, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorStartCPUs(priv->mon, conn); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } if (ret == 0) { vm->state = VIR_DOMAIN_RUNNING; } @@ -1908,7 +1933,11 @@ int qemuProcessStopCPUs(struct qemud_driver *driver, virDomainObjPtr vm) vm->state = VIR_DOMAIN_PAUSED; qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorStopCPUs(priv->mon); - qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + } if (ret < 0) { vm->state = oldState; } @@ -2389,11 +2418,15 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG0("Setting initial memory amount"); cur_balloon = vm->def->mem.cur_balloon; qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; + ret = qemuMonitorSetBalloon(priv->mon, cur_balloon); + if (qemuDomainObjExitMonitorWithDriver(driver, vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; } - qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (ret < 0) + goto cleanup; if (!start_paused) { VIR_DEBUG0("Starting domain CPUs"); -- 1.7.1
On Mon, Apr 11, 2011 at 01:41:55PM +0800, Wen Congyang wrote:
qemu may quited unexpectedly when invoking a monitor command. And priv->mon will be NULL after qemuDomainObjExitMonitor* returns. So we must not use it. Unfortunately we still use it, and it will cause libvirtd crashed.
As Eric suggested, qemuDomainObjExitMonitor* should be made to return the value of vm active after regaining lock, and marked ATTRIBUTE_RETURN_CHECK, to force all other callers to detect the case of a monitor command completing successfully but then the VM disappearing in the window between command completion and regaining the vm lock.
@@ -578,7 +578,7 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) * * Should be paired with an earlier qemuDomainObjEnterMonitor() call */ -void qemuDomainObjExitMonitor(virDomainObjPtr obj) +int qemuDomainObjExitMonitor(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; int refs; @@ -588,11 +588,20 @@ void qemuDomainObjExitMonitor(virDomainObjPtr obj) if (refs > 0) qemuMonitorUnlock(priv->mon);
+ /* Note: qemu may quited unexpectedly here, and the monitor will be freed. + * If it happened, priv->mon will be null. + */ + virDomainObjLock(obj);
if (refs == 0) { priv->mon = NULL; } + + if (priv->mon == NULL) + return -1;
Perhaps here we should do qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("guest unexpectedly quit"));
+ else + return 0; }
@@ -621,8 +630,8 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, * * Should be paired with an earlier qemuDomainObjEnterMonitorWithDriver() call */ -void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, - virDomainObjPtr obj) +int qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, + virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; int refs; @@ -632,12 +641,21 @@ void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, if (refs > 0) qemuMonitorUnlock(priv->mon);
+ /* Note: qemu may quited unexpectedly here, and the monitor will be freed. + * If it happened, priv->mon will be null. + */ + qemuDriverLock(driver); virDomainObjLock(obj);
if (refs == 0) { priv->mon = NULL; } + + if (priv->mon == NULL) + return -1;
And the same here
+ else + return 0; }
@@ -1452,7 +1452,11 @@ static int qemudDomainShutdown(virDomainPtr dom) { priv = vm->privateData; qemuDomainObjEnterMonitor(vm); ret = qemuMonitorSystemPowerdown(priv->mon); - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + }
So we can avoid qemuReportError() here, and in every single other place where ExitMonitor is called. It would make the code a bit shorter Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Wen Congyang