9:42 a.m.
Patches 3-8 fix possible crash/invalid memory access if QEMU crashes
while we're in the monitor.
Patches 9-13 change all other callers of qemuDomainObjExitMonitor
to exit early in that case, but should not fix any real issues.
They are necessary to turn on ATTRIBUTE_RETURN_CHECK for the ExitMonitor
call.
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
Ján Tomko (14):
Check for domain liveness in qemuDomainObjExitMonitor
Mark the domain as active in qemuhotplugtest
Fix vmdef usage after domain crash in monitor on device removal
Fix vmdef usage after domain crash in monitor on device detach
Fix vmdef usage after domain crash in monitor on device attach
Fix vmdef usage while in monitor in qemuDomainHotplugVcpus
Fix vmdef usage while in monitor in BlockStat* APIs
Fix vmdef usage while in monitor in qemu process
Exit early after domain crash in monitor on device hotplug
Exit early after domain crash in monitor on migration
Exit early after domain crash in monitor in qemu_process
Exit early after domain crash in monitor in qemu_driver
Exit early after domain crash in monitor on snapshots
Add ATTRIBUTE_RETURN_CHECK to qemuDomainObjExitMonitor
src/qemu/THREADS.txt | 5 ++
src/qemu/qemu_domain.c | 27 +++++--
src/qemu/qemu_domain.h | 7 +-
src/qemu/qemu_driver.c | 196 +++++++++++++++++++++++++++++-----------------
src/qemu/qemu_hotplug.c | 183 ++++++++++++++++++++++++++-----------------
src/qemu/qemu_hotplug.h | 6 +-
src/qemu/qemu_migration.c | 137 ++++++++++++++++----------------
src/qemu/qemu_process.c | 128 +++++++++++++++++-------------
tests/qemuhotplugtest.c | 6 ++
9 files changed, 410 insertions(+), 285 deletions(-)
--
2.0.4
9:42 a.m.
New subject: [libvirt] [PATCHv2 01/14] Check for domain liveness in qemuDomainObjExitMonitor
The domain might disappear during the time in monitor when
the virDomainObjPtr is unlocked, so the caller needs to check
if it's still alive.
Since most of the callers are going to need it, put the
check inside qemuDomainObjExitMonitor and return -1 if
the domain died in the meantime.
---
src/qemu/THREADS.txt | 5 +++++
src/qemu/qemu_domain.c | 16 ++++++++++++++--
src/qemu/qemu_domain.h | 4 ++--
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
index add2a35..dfa372b 100644
--- a/src/qemu/THREADS.txt
+++ b/src/qemu/THREADS.txt
@@ -160,6 +160,11 @@ To acquire the QEMU monitor lock
- Acquires the virDomainObjPtr lock
These functions must not be used by an asynchronous job.
+ Note that the virDomainObj is unlocked during the time in
+ monitor and it can be changed, e.g. if QEMU dies, qemuProcessStop
+ may free the live domain definition and put the persistent
+ definition back in vm->def. The callers should check the return
+ value of ExitMonitor to see if the domain is still alive.
To acquire the QEMU monitor lock as part of an asynchronous job
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3d4023c..c942b02 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1586,11 +1586,23 @@ void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver,
/* obj must NOT be locked before calling
*
* Should be paired with an earlier qemuDomainObjEnterMonitor() call
+ *
+ * Returns -1 if the domain is no longer alive after exiting the monitor.
+ * In that case, the caller should be careful when using obj's data,
+ * e.g. the live definition in vm->def has been freed by qemuProcessStop
+ * and replaced by the persistent definition, so pointers stolen
+ * from the live definition could no longer be valid.
*/
-void qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
- virDomainObjPtr obj)
+int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
+ virDomainObjPtr obj)
{
qemuDomainObjExitMonitorInternal(driver, obj);
+ if (!virDomainObjIsActive(obj)) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("domain is no longer running"));
+ return -1;
+ }
+ return 0;
}
/*
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 6b52f03..fd91d83 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -245,8 +245,8 @@ void qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj);
void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver,
virDomainObjPtr obj)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-void qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
- virDomainObjPtr obj)
+int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
+ virDomainObjPtr obj)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
virDomainObjPtr obj,
--
2.0.4
1:02 p.m.
New subject: [libvirt] [PATCHv2 01/14] Check for domain liveness in qemuDomainObjExitMonitor
On 01/07/2015 10:42 AM, Ján Tomko wrote:
The domain might disappear during the time in monitor when
the virDomainObjPtr is unlocked, so the caller needs to check
if it's still alive.
Since most of the callers are going to need it, put the
check inside qemuDomainObjExitMonitor and return -1 if
the domain died in the meantime.
---
src/qemu/THREADS.txt | 5 +++++
src/qemu/qemu_domain.c | 16 ++++++++++++++--
src/qemu/qemu_domain.h | 4 ++--
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
index add2a35..dfa372b 100644
--- a/src/qemu/THREADS.txt
+++ b/src/qemu/THREADS.txt
@@ -160,6 +160,11 @@ To acquire the QEMU monitor lock
- Acquires the virDomainObjPtr lock
These functions must not be used by an asynchronous job.
+ Note that the virDomainObj is unlocked during the time in
+ monitor and it can be changed, e.g. if QEMU dies, qemuProcessStop
+ may free the live domain definition and put the persistent
+ definition back in vm->def. The callers should check the return
+ value of ExitMonitor to see if the domain is still alive.
To acquire the QEMU monitor lock as part of an asynchronous job
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3d4023c..c942b02 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1586,11 +1586,23 @@ void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver,
/* obj must NOT be locked before calling
*
* Should be paired with an earlier qemuDomainObjEnterMonitor() call
+ *
+ * Returns -1 if the domain is no longer alive after exiting the monitor.
+ * In that case, the caller should be careful when using obj's data,
+ * e.g. the live definition in vm->def has been freed by qemuProcessStop
+ * and replaced by the persistent definition, so pointers stolen
+ * from the live definition could no longer be valid.
*/
-void qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
- virDomainObjPtr obj)
+int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
+ virDomainObjPtr obj)
{
qemuDomainObjExitMonitorInternal(driver, obj);
+ if (!virDomainObjIsActive(obj)) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("domain is no longer running"));
+ return -1;
+ }
+ return 0;
Do we have to worry about caller paths perhaps using virReportError and
this overwriting the other message for the last message? Does it really
matter?
ACK
John
}
/*
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 6b52f03..fd91d83 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -245,8 +245,8 @@ void qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj);
void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver,
virDomainObjPtr obj)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-void qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
- virDomainObjPtr obj)
+int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
+ virDomainObjPtr obj)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
virDomainObjPtr obj,
6:38 a.m.
New subject: [libvirt] [PATCHv2 01/14] Check for domain liveness in qemuDomainObjExitMonitor
On 01/12/2015 08:02 PM, John Ferlan wrote:
On 01/07/2015 10:42 AM, Ján Tomko wrote:
> The domain might disappear during the time in monitor when
> the virDomainObjPtr is unlocked, so the caller needs to check
> if it's still alive.
>
> Since most of the callers are going to need it, put the
> check inside qemuDomainObjExitMonitor and return -1 if
> the domain died in the meantime.
> ---
> src/qemu/THREADS.txt | 5 +++++
> src/qemu/qemu_domain.c | 16 ++++++++++++++--
> src/qemu/qemu_domain.h | 4 ++--
> 3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
> index add2a35..dfa372b 100644
> --- a/src/qemu/THREADS.txt
> +++ b/src/qemu/THREADS.txt
> @@ -160,6 +160,11 @@ To acquire the QEMU monitor lock
> - Acquires the virDomainObjPtr lock
>
> These functions must not be used by an asynchronous job.
> + Note that the virDomainObj is unlocked during the time in
> + monitor and it can be changed, e.g. if QEMU dies, qemuProcessStop
> + may free the live domain definition and put the persistent
> + definition back in vm->def. The callers should check the return
> + value of ExitMonitor to see if the domain is still alive.
>
>
> To acquire the QEMU monitor lock as part of an asynchronous job
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3d4023c..c942b02 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1586,11 +1586,23 @@ void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver,
> /* obj must NOT be locked before calling
> *
> * Should be paired with an earlier qemuDomainObjEnterMonitor() call
> + *
> + * Returns -1 if the domain is no longer alive after exiting the monitor.
> + * In that case, the caller should be careful when using obj's data,
> + * e.g. the live definition in vm->def has been freed by qemuProcessStop
> + * and replaced by the persistent definition, so pointers stolen
> + * from the live definition could no longer be valid.
> */
> -void qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
> - virDomainObjPtr obj)
> +int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
> + virDomainObjPtr obj)
> {
> qemuDomainObjExitMonitorInternal(driver, obj);
> + if (!virDomainObjIsActive(obj)) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("domain is no longer running"));
> + return -1;
> + }
> + return 0;
Do we have to worry about caller paths perhaps using virReportError and
this overwriting the other message for the last message? Does it really
matter?
Well, we'd overwrite 'end of file from monitor' by 'domain is no longer
running' and personally I don't think it matters.
Jan
ACK
John
9:42 a.m.
New subject: [libvirt] [PATCHv2 02/14] Mark the domain as active in qemuhotplugtest
This will allow us to call qemuDomainObjIsActive() in
the tested functions to check if the domain has crashed.
---
tests/qemuhotplugtest.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 9d39968..f0517a1 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -40,6 +40,8 @@ enum {
UPDATE
};
+#define QEMU_HOTPLUG_TEST_DOMAIN_ID 7
+
struct qemuHotplugTestData {
const char *domain_filename;
const char *device_filename;
@@ -90,6 +92,8 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0)
goto cleanup;
+ (*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
+
ret = 0;
cleanup:
return ret;
@@ -177,9 +181,11 @@ testQemuHotplugCheckResult(virDomainObjPtr vm,
char *actual;
int ret;
+ vm->def->id = -1;
actual = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE);
if (!actual)
return -1;
+ vm->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
if (STREQ(expected, actual)) {
if (fail && virTestGetVerbose())
--
2.0.4
1:03 p.m.
New subject: [libvirt] [PATCHv2 02/14] Mark the domain as active in qemuhotplugtest
On 01/07/2015 10:42 AM, Ján Tomko wrote:
This will allow us to call qemuDomainObjIsActive() in
the tested functions to check if the domain has crashed.
---
tests/qemuhotplugtest.c | 6 ++++++
1 file changed, 6 insertions(+)
ACK
John
9:42 a.m.
New subject: [libvirt] [PATCHv2 03/14] Fix vmdef usage after domain crash in monitor on device removal
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
In the device type-specific functions, exit early
if the domain has disappeared, because the cleanup
should have been done by qemuProcessStop.
Check the return value in processDeviceDeletedEvent
and qemuProcessUpdateDevices.
---
src/qemu/qemu_driver.c | 3 ++-
src/qemu/qemu_hotplug.c | 32 ++++++++++++++++++++------------
src/qemu/qemu_hotplug.h | 6 +++---
src/qemu/qemu_process.c | 6 ++++--
4 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cdf4173..f7c9219 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4044,7 +4044,8 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0)
goto endjob;
- qemuDomainRemoveDevice(driver, vm, &dev);
+ if (qemuDomainRemoveDevice(driver, vm, &dev) < 0)
+ goto endjob;
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
VIR_WARN("unable to save domain status after removing device %s",
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1714341..ce05f80 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2491,8 +2491,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorDriveDel(priv->mon, drivestr);
- qemuDomainObjExitMonitor(driver, vm);
VIR_FREE(drivestr);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
virDomainAuditDisk(vm, disk->src, NULL, "detach", true);
@@ -2607,7 +2608,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorDriveDel(priv->mon, drivestr);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
}
event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias);
@@ -2701,7 +2703,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditNet(vm, net, NULL, "detach", false);
goto cleanup;
}
@@ -2713,12 +2716,14 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("unable to determine original VLAN"));
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditNet(vm, net, NULL, "detach", false);
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditNet(vm, net, NULL, "detach", true);
@@ -2788,7 +2793,8 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditChardev(vm, chr, NULL, "detach", rc == 0);
@@ -2809,27 +2815,28 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
}
-void
+int
qemuDomainRemoveDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
{
+ int ret = -1;
switch ((virDomainDeviceType) dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
- qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk);
+ ret = qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk);
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller);
+ ret = qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller);
break;
case VIR_DOMAIN_DEVICE_NET:
- qemuDomainRemoveNetDevice(driver, vm, dev->data.net);
+ ret = qemuDomainRemoveNetDevice(driver, vm, dev->data.net);
break;
case VIR_DOMAIN_DEVICE_HOSTDEV:
- qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev);
+ ret = qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev);
break;
case VIR_DOMAIN_DEVICE_CHR:
- qemuDomainRemoveChrDevice(driver, vm, dev->data.chr);
+ ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr);
break;
case VIR_DOMAIN_DEVICE_NONE:
@@ -2855,6 +2862,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
virDomainDeviceTypeToString(dev->type));
break;
}
+ return ret;
}
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index d13c532..19ab9a0 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -107,9 +107,9 @@ virDomainChrDefPtr
qemuDomainChrRemove(virDomainDefPtr vmdef,
virDomainChrDefPtr chr);
-void qemuDomainRemoveDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev);
+int qemuDomainRemoveDevice(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev);
bool qemuDomainSignalDeviceRemoval(virDomainObjPtr vm,
const char *devAlias);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c18204b..4528b58 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3547,8 +3547,10 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver,
if ((tmp = old)) {
while (*tmp) {
if (!virStringArrayHasString(priv->qemuDevices, *tmp) &&
- virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0)
- qemuDomainRemoveDevice(driver, vm, &dev);
+ virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0
&&
+ qemuDomainRemoveDevice(driver, vm, &dev) < 0) {
+ goto cleanup;
+ }
tmp++;
}
}
--
2.0.4
3:44 p.m.
New subject: [libvirt] [PATCHv2 03/14] Fix vmdef usage after domain crash in monitor on device removal
On 01/07/2015 10:42 AM, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
In the device type-specific functions, exit early
if the domain has disappeared, because the cleanup
should have been done by qemuProcessStop.
Check the return value in processDeviceDeletedEvent
and qemuProcessUpdateDevices.
---
src/qemu/qemu_driver.c | 3 ++-
src/qemu/qemu_hotplug.c | 32 ++++++++++++++++++++------------
src/qemu/qemu_hotplug.h | 6 +++---
src/qemu/qemu_process.c | 6 ++++--
4 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cdf4173..f7c9219 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4044,7 +4044,8 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0)
goto endjob;
- qemuDomainRemoveDevice(driver, vm, &dev);
+ if (qemuDomainRemoveDevice(driver, vm, &dev) < 0)
+ goto endjob;
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
VIR_WARN("unable to save domain status after removing device %s",
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1714341..ce05f80 100644
The following generally applies to patches 3-5...
For hotplug, the ExitMonitor failures are done prior to auditing or
sending events. Although not all the code handles failures and auditing
quite the same - I would think we still want to Audit the qemuMonitor*
calls regardless of whether the process dies or not. The audit code uses
vm->def->{uuid|name|virtType} to format the message it generates, so is
that "safe" according to what happens on the ExitMonitor failure?
One reason this springs to mind is some of the code will Audit on the
qemuMonitor* call success/failure and I would think perhaps one of the
failures is that the vm died and having that Audit trail could be a good
thing. Beyond that knowing that the qemuMonitor* call passed (because
Audit told us so), but then finding the vm crashed (because ExitMonitor
told us so) could narrow some other bizarre window.
So while perhaps slightly outside the scope of these changes - what
affect does exiting prior to Audit really have... and yes, event is
different can of worms.
As I go deeper into the patches I see the HotplugVcpus will call
virDomainAuditVcpu even after the ignored ExitMonitor, so it seems safe...
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2491,8 +2491,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorDriveDel(priv->mon, drivestr);
- qemuDomainObjExitMonitor(driver, vm);
VIR_FREE(drivestr);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
virDomainAuditDisk(vm, disk->src, NULL, "detach", true);
Missed audit call... I think the last parameter should true/false based
on whether DriveDel succeeds, right? Although I do have a recollection
of some interaction with DelDevice while working through the hostdev
changes.
@@ -2607,7 +2608,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorDriveDel(priv->mon, drivestr);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
}
hmm... the Audit call quite a few lines later and it assumes success.
event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias);
@@ -2701,7 +2703,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditNet(vm, net, NULL, "detach", false);
goto cleanup;
}
@@ -2713,12 +2716,14 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("unable to determine original VLAN"));
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
NOTE: There's a virReportError before us in this path...
virDomainAuditNet(vm, net, NULL, "detach",
false);
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
For each of these 3 changes - the AuditNet will not occur...
virDomainAuditNet(vm, net, NULL, "detach", true);
@@ -2788,7 +2793,8 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
Missed Audit call
Weak ACK for what's here and if it's felt Auditing isn't necessary, then
I have no other objections...
John
virDomainAuditChardev(vm, chr, NULL, "detach", rc ==
0);
@@ -2809,27 +2815,28 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
}
-void
+int
qemuDomainRemoveDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
{
+ int ret = -1;
switch ((virDomainDeviceType) dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
- qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk);
+ ret = qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk);
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller);
+ ret = qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller);
break;
case VIR_DOMAIN_DEVICE_NET:
- qemuDomainRemoveNetDevice(driver, vm, dev->data.net);
+ ret = qemuDomainRemoveNetDevice(driver, vm, dev->data.net);
break;
case VIR_DOMAIN_DEVICE_HOSTDEV:
- qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev);
+ ret = qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev);
break;
case VIR_DOMAIN_DEVICE_CHR:
- qemuDomainRemoveChrDevice(driver, vm, dev->data.chr);
+ ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr);
break;
case VIR_DOMAIN_DEVICE_NONE:
@@ -2855,6 +2862,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
virDomainDeviceTypeToString(dev->type));
break;
}
+ return ret;
}
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index d13c532..19ab9a0 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -107,9 +107,9 @@ virDomainChrDefPtr
qemuDomainChrRemove(virDomainDefPtr vmdef,
virDomainChrDefPtr chr);
-void qemuDomainRemoveDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev);
+int qemuDomainRemoveDevice(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev);
bool qemuDomainSignalDeviceRemoval(virDomainObjPtr vm,
const char *devAlias);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c18204b..4528b58 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3547,8 +3547,10 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver,
if ((tmp = old)) {
while (*tmp) {
if (!virStringArrayHasString(priv->qemuDevices, *tmp) &&
- virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0)
- qemuDomainRemoveDevice(driver, vm, &dev);
+ virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0
&&
+ qemuDomainRemoveDevice(driver, vm, &dev) < 0) {
+ goto cleanup;
+ }
tmp++;
}
}
6:38 a.m.
New subject: [libvirt] [PATCHv2 03/14] Fix vmdef usage after domain crash in monitor on device removal
On 01/12/2015 10:44 PM, John Ferlan wrote:
On 01/07/2015 10:42 AM, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1161024
>
> In the device type-specific functions, exit early
> if the domain has disappeared, because the cleanup
> should have been done by qemuProcessStop.
>
> Check the return value in processDeviceDeletedEvent
> and qemuProcessUpdateDevices.
> ---
> src/qemu/qemu_driver.c | 3 ++-
> src/qemu/qemu_hotplug.c | 32 ++++++++++++++++++++------------
> src/qemu/qemu_hotplug.h | 6 +++---
> src/qemu/qemu_process.c | 6 ++++--
> 4 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cdf4173..f7c9219 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4044,7 +4044,8 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
> if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0)
> goto endjob;
>
> - qemuDomainRemoveDevice(driver, vm, &dev);
> + if (qemuDomainRemoveDevice(driver, vm, &dev) < 0)
> + goto endjob;
>
> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> VIR_WARN("unable to save domain status after removing device %s",
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1714341..ce05f80 100644
The following generally applies to patches 3-5...
For hotplug, the ExitMonitor failures are done prior to auditing or
sending events. Although not all the code handles failures and auditing
quite the same - I would think we still want to Audit the qemuMonitor*
calls regardless of whether the process dies or not. The audit code uses
vm->def->{uuid|name|virtType} to format the message it generates, so is
that "safe" according to what happens on the ExitMonitor failure?
One reason this springs to mind is some of the code will Audit on the
qemuMonitor* call success/failure and I would think perhaps one of the
failures is that the vm died and having that Audit trail could be a good
thing. Beyond that knowing that the qemuMonitor* call passed (because
Audit told us so), but then finding the vm crashed (because ExitMonitor
told us so) could narrow some other bizarre window.
So while perhaps slightly outside the scope of these changes - what
affect does exiting prior to Audit really have... and yes, event is
different can of worms.
As I go deeper into the patches I see the HotplugVcpus will call
virDomainAuditVcpu even after the ignored ExitMonitor, so it seems safe...
For HotplugVcpus, we only pass the number of vcpus to the audit function. For
hotplug, the Attach* function is the one owning the pointer to the device
(except for the chardev attach function), so we can safely audit that.
The skipped audits are those using pointers that are in the vm->def and could
be freed by qemuProcessStop in another thread in the meantime. I thought the
domain crash audit in there would be enough.
Jan
9:42 a.m.
New subject: [libvirt] [PATCHv2 04/14] Fix vmdef usage after domain crash in monitor on device detach
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
Skip audit and removing the device from live def because
it has already been cleaned up.
---
src/qemu/qemu_hotplug.c | 59 ++++++++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ce05f80..c480dcd 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2986,19 +2986,22 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
goto cleanup;
}
} else {
if (qemuMonitorRemovePCIDevice(priv->mon,
&detach->info.addr.pci) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3038,11 +3041,13 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
goto cleanup;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3219,17 +3224,20 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
goto cleanup;
}
} else {
if (qemuMonitorRemovePCIDevice(priv->mon,
&detach->info.addr.pci) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3274,7 +3282,8 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver,
} else {
ret = qemuMonitorRemovePCIDevice(priv->mon,
&detach->info->addr.pci);
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
return ret;
}
@@ -3303,7 +3312,8 @@ qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
return ret;
}
@@ -3331,14 +3341,11 @@ qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver,
qemuDomainMarkDeviceForRemoval(vm, detach->info);
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, detach->info->alias) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
- }
- qemuDomainObjExitMonitor(driver, vm);
- ret = 0;
+ ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
- cleanup:
return ret;
}
@@ -3374,7 +3381,8 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
}
if (ret < 0) {
- virDomainAuditHostdev(vm, detach, "detach", false);
+ if (virDomainObjIsActive(vm))
+ virDomainAuditHostdev(vm, detach, "detach", false);
} else {
int rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3530,19 +3538,22 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditNet(vm, detach, NULL, "detach", false);
goto cleanup;
}
} else {
if (qemuMonitorRemovePCIDevice(priv->mon,
&detach->info.addr.pci) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditNet(vm, detach, NULL, "detach", false);
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3709,10 +3720,12 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) <
0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
goto cleanup;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
--
2.0.4
3:58 p.m.
New subject: [libvirt] [PATCHv2 04/14] Fix vmdef usage after domain crash in monitor on device detach
On 01/07/2015 10:42 AM, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
Skip audit and removing the device from live def because
it has already been cleaned up.
---
src/qemu/qemu_hotplug.c | 59 ++++++++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 23 deletions(-)
Similar Audit concerns w/ 3/14...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ce05f80..c480dcd 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2986,19 +2986,22 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
goto cleanup;
}
} else {
if (qemuMonitorRemovePCIDevice(priv->mon,
&detach->info.addr.pci) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3038,11 +3041,13 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
goto cleanup;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3219,17 +3224,20 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
goto cleanup;
}
} else {
if (qemuMonitorRemovePCIDevice(priv->mon,
&detach->info.addr.pci) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3274,7 +3282,8 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver,
} else {
ret = qemuMonitorRemovePCIDevice(priv->mon,
&detach->info->addr.pci);
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
return ret;
Or just ret qemuDomainObjExitMonitor(driver, vm);
}
@@ -3303,7 +3312,8 @@ qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
Or just ret qemuDomainObjExitMonitor(driver, vm);
return ret;
}
@@ -3331,14 +3341,11 @@ qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver,
qemuDomainMarkDeviceForRemoval(vm, detach->info);
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, detach->info->alias) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
- }
- qemuDomainObjExitMonitor(driver, vm);
- ret = 0;
+ ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
- cleanup:
return ret;
}
@@ -3374,7 +3381,8 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
}
if (ret < 0) {
- virDomainAuditHostdev(vm, detach, "detach", false);
+ if (virDomainObjIsActive(vm))
+ virDomainAuditHostdev(vm, detach, "detach", false);
Hmm.... well this makes my comments in 3/14 appear to be unnecessary,
since there's an explicit check for active vm... Although the Vcpu
failure will still call virDomainAuditVcpu
ACK based on whether it's felt Auditing is necessary or not.
John
} else {
int rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3530,19 +3538,22 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditNet(vm, detach, NULL, "detach", false);
goto cleanup;
}
} else {
if (qemuMonitorRemovePCIDevice(priv->mon,
&detach->info.addr.pci) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
virDomainAuditNet(vm, detach, NULL, "detach", false);
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
@@ -3709,10 +3720,12 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) <
0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
goto cleanup;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
rc = qemuDomainWaitForDeviceRemoval(vm);
if (rc == 0 || rc == 1)
6:38 a.m.
New subject: [libvirt] [PATCHv2 04/14] Fix vmdef usage after domain crash in monitor on device detach
On 01/12/2015 10:58 PM, John Ferlan wrote:
On 01/07/2015 10:42 AM, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1161024
>
> Skip audit and removing the device from live def because
> it has already been cleaned up.
> ---
> src/qemu/qemu_hotplug.c | 59 ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 36 insertions(+), 23 deletions(-)
>
Similar Audit concerns w/ 3/14...
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index ce05f80..c480dcd 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2986,19 +2986,22 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
> qemuDomainObjEnterMonitor(driver, vm);
> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> virDomainAuditDisk(vm, detach->src, NULL, "detach",
false);
> goto cleanup;
> }
> } else {
> if (qemuMonitorRemovePCIDevice(priv->mon,
> &detach->info.addr.pci) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> virDomainAuditDisk(vm, detach->src, NULL, "detach",
false);
> goto cleanup;
> }
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
>
> rc = qemuDomainWaitForDeviceRemoval(vm);
> if (rc == 0 || rc == 1)
> @@ -3038,11 +3041,13 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
>
> qemuDomainObjEnterMonitor(driver, vm);
> if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
> goto cleanup;
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
>
> rc = qemuDomainWaitForDeviceRemoval(vm);
> if (rc == 0 || rc == 1)
> @@ -3219,17 +3224,20 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr
driver,
> qemuDomainObjEnterMonitor(driver, vm);
> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> goto cleanup;
> }
> } else {
> if (qemuMonitorRemovePCIDevice(priv->mon,
> &detach->info.addr.pci) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> goto cleanup;
> }
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
>
> rc = qemuDomainWaitForDeviceRemoval(vm);
> if (rc == 0 || rc == 1)
> @@ -3274,7 +3282,8 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver,
> } else {
> ret = qemuMonitorRemovePCIDevice(priv->mon,
&detach->info->addr.pci);
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> return ret;
Or just ret qemuDomainObjExitMonitor(driver, vm);
That would return 0 in the case of RemovePCIDevice failing, but not crashing
the domain.
> @@ -3374,7 +3381,8 @@
qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
> }
>
> if (ret < 0) {
> - virDomainAuditHostdev(vm, detach, "detach", false);
> + if (virDomainObjIsActive(vm))
> + virDomainAuditHostdev(vm, detach, "detach", false);
Hmm.... well this makes my comments in 3/14 appear to be unnecessary,
since there's an explicit check for active vm... Although the Vcpu
failure will still call virDomainAuditVcpu
'detach' here is a pointer to the device in the vm->def, so it could be freed
in the meantime if the domain crashed.
Jan
9:42 a.m.
New subject: [libvirt] [PATCHv2 05/14] Fix vmdef usage after domain crash in monitor on device attach
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
If the domain crashed while we were in monitor,
we cannot rely on the REALLOC done on live definition,
since vm->def now points to the persistent definition.
Skip adding the attached devices to domain definition
if the domain crashed.
In AttachChrDevice, the chardev was already added to the
live definition and freed by qemuProcessStop in the case
of a crash. Skip the device removal in that case.
---
src/qemu/qemu_hotplug.c | 54 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c480dcd..a4e4d6b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -391,7 +391,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
memcpy(&disk->info.addr.pci, &guestAddr, sizeof(guestAddr));
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ releaseaddr = false;
+ ret = -1;
+ }
virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);
@@ -477,7 +480,10 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
type,
&controller->info.addr.pci);
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ releaseaddr = false;
+ ret = -1;
+ }
if (ret == 0) {
if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
@@ -628,7 +634,8 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
disk->info.addr.drive.unit = driveAddr.unit;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);
@@ -708,7 +715,8 @@ qemuDomainAttachUSBMassstorageDevice(virConnectPtr conn,
} else {
ret = qemuMonitorAddUSBDisk(priv->mon, src);
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);
@@ -1272,7 +1280,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
configfd, configfd_name);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
} else {
virDevicePCIAddressPtr guestAddr = &hostdev->info->addr.pci;
virDevicePCIAddressPtr hostAddr = &hostdev->source.subsys.u.pci.addr;
@@ -1288,7 +1297,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorAddPCIHostDevice(priv->mon, hostAddr, guestAddr);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
}
@@ -1352,12 +1362,11 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
goto error;
qemuDomainObjEnterMonitor(driver, vm);
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE))
- ret = qemuMonitorAddDevice(priv->mon, devstr);
- else
- goto error;
+ ret = qemuMonitorAddDevice(priv->mon, devstr);
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
- qemuDomainObjExitMonitor(driver, vm);
virDomainAuditRedirdev(vm, redirdev, "attach", ret == 0);
if (ret < 0)
goto error;
@@ -1479,17 +1488,22 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ need_remove = false;
goto audit;
}
if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) {
/* detach associated chardev on error */
qemuMonitorDetachCharDev(priv->mon, charAlias);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ need_remove = false;
+ goto audit;
+ }
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ need_remove = false;
goto audit;
}
- qemuDomainObjExitMonitor(driver, vm);
ret = 0;
audit:
@@ -1545,7 +1559,8 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
ret = qemuMonitorAddUSBDeviceExact(priv->mon,
hostdev->source.subsys.u.usb.bus,
hostdev->source.subsys.u.usb.device);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
virDomainAuditHostdev(vm, hostdev, "attach", ret == 0);
if (ret < 0)
goto cleanup;
@@ -1648,7 +1663,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
}
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
virDomainAuditHostdev(vm, hostdev, "attach", ret == 0);
if (ret < 0)
@@ -1848,7 +1864,8 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
dev->linkstate = linkstate;
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
return ret;
}
@@ -3633,7 +3650,8 @@ qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver,
}
end_job:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
cleanup:
virObjectUnref(cfg);
return ret;
--
2.0.4
4:08 p.m.
New subject: [libvirt] [PATCHv2 05/14] Fix vmdef usage after domain crash in monitor on device attach
On 01/07/2015 10:42 AM, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
If the domain crashed while we were in monitor,
we cannot rely on the REALLOC done on live definition,
since vm->def now points to the persistent definition.
Skip adding the attached devices to domain definition
if the domain crashed.
In AttachChrDevice, the chardev was already added to the
live definition and freed by qemuProcessStop in the case
of a crash. Skip the device removal in that case.
---
src/qemu/qemu_hotplug.c | 54 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c480dcd..a4e4d6b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -391,7 +391,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
memcpy(&disk->info.addr.pci, &guestAddr, sizeof(guestAddr));
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ releaseaddr = false;
+ ret = -1;
+ }
So here's another case where will will do the Audit even though
ExitMonitor fails - seems to contract the check in 4/14...
virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);
@@ -477,7 +480,10 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
type,
&controller->info.addr.pci);
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ releaseaddr = false;
+ ret = -1;
+ }
No Audit for AttachController, but detach and remove don't have one
either...
if (ret == 0) {
if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
@@ -628,7 +634,8 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
disk->info.addr.drive.unit = driveAddr.unit;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
Again here we do the Audit...
virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);
@@ -708,7 +715,8 @@ qemuDomainAttachUSBMassstorageDevice(virConnectPtr conn,
} else {
ret = qemuMonitorAddUSBDisk(priv->mon, src);
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
Again the Audit is done
virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0);
@@ -1272,7 +1280,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
configfd, configfd_name);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
after the else is the Audit
} else {
virDevicePCIAddressPtr guestAddr = &hostdev->info->addr.pci;
virDevicePCIAddressPtr hostAddr = &hostdev->source.subsys.u.pci.addr;
@@ -1288,7 +1297,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorAddPCIHostDevice(priv->mon, hostAddr, guestAddr);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
same function, same Audit
hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
}
@@ -1352,12 +1362,11 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
goto error;
qemuDomainObjEnterMonitor(driver, vm);
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE))
- ret = qemuMonitorAddDevice(priv->mon, devstr);
- else
- goto error;
+ ret = qemuMonitorAddDevice(priv->mon, devstr);
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
Here we Audit again
- qemuDomainObjExitMonitor(driver, vm);
virDomainAuditRedirdev(vm, redirdev, "attach", ret == 0);
if (ret < 0)
goto error;
@@ -1479,17 +1488,22 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0)
{
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ need_remove = false;
goto audit;
Auditing here too.
}
if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) {
/* detach associated chardev on error */
qemuMonitorDetachCharDev(priv->mon, charAlias);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ need_remove = false;
+ goto audit;
+ }
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ need_remove = false;
goto audit;
Same...
}
- qemuDomainObjExitMonitor(driver, vm);
ret = 0;
audit:
@@ -1545,7 +1559,8 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
ret = qemuMonitorAddUSBDeviceExact(priv->mon,
hostdev->source.subsys.u.usb.bus,
hostdev->source.subsys.u.usb.device);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
Same
virDomainAuditHostdev(vm, hostdev, "attach", ret ==
0);
if (ret < 0)
goto cleanup;
@@ -1648,7 +1663,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
}
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
Same
John
virDomainAuditHostdev(vm, hostdev, "attach", ret == 0);
if (ret < 0)
@@ -1848,7 +1864,8 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
dev->linkstate = linkstate;
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
return ret;
}
@@ -3633,7 +3650,8 @@ qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver,
}
end_job:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
cleanup:
virObjectUnref(cfg);
return ret;
9:42 a.m.
New subject: [libvirt] [PATCHv2 06/14] Fix vmdef usage while in monitor in qemuDomainHotplugVcpus
Exit the monitor right after we've done with it to get
the virDomainObjPtr lock back, otherwise we might be accessing
vm->def while it's being cleaned up by qemuProcessStop.
If the domain crashed while we were in the monitor, exit
early instead of changing vm->def which is now the persistent
definition.
---
src/qemu/qemu_driver.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f7c9219..1275ba4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4367,7 +4367,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
if (rc == 0)
goto unsupported;
if (rc < 0)
- goto cleanup;
+ goto exit_monitor;
vcpus++;
}
@@ -4378,7 +4378,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
if (rc == 0)
goto unsupported;
if (rc < 0)
- goto cleanup;
+ goto exit_monitor;
vcpus--;
}
@@ -4395,6 +4395,10 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
* fatal */
if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
virResetLastError();
+ goto exit_monitor;
+ }
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
goto cleanup;
}
@@ -4515,10 +4519,10 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
cpupids = NULL;
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
- vm->def->vcpus = vcpus;
VIR_FREE(cpupids);
VIR_FREE(mem_mask);
+ if (virDomainObjIsActive(vm))
+ vm->def->vcpus = vcpus;
virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);
if (cgroup_vcpu)
virCgroupFree(&cgroup_vcpu);
@@ -4527,6 +4531,8 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
unsupported:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot change vcpu count of this domain"));
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto cleanup;
}
--
2.0.4
4:10 p.m.
New subject: [libvirt] [PATCHv2 06/14] Fix vmdef usage while in monitor in qemuDomainHotplugVcpus
On 01/07/2015 10:42 AM, Ján Tomko wrote:
Exit the monitor right after we've done with it to get
the virDomainObjPtr lock back, otherwise we might be accessing
vm->def while it's being cleaned up by qemuProcessStop.
If the domain crashed while we were in the monitor, exit
early instead of changing vm->def which is now the persistent
definition.
---
src/qemu/qemu_driver.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f7c9219..1275ba4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4367,7 +4367,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
if (rc == 0)
goto unsupported;
if (rc < 0)
- goto cleanup;
+ goto exit_monitor;
vcpus++;
}
@@ -4378,7 +4378,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
if (rc == 0)
goto unsupported;
if (rc < 0)
- goto cleanup;
+ goto exit_monitor;
vcpus--;
}
@@ -4395,6 +4395,10 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
* fatal */
if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
virResetLastError();
+ goto exit_monitor;
+ }
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
goto cleanup;
}
@@ -4515,10 +4519,10 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
cpupids = NULL;
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
- vm->def->vcpus = vcpus;
VIR_FREE(cpupids);
VIR_FREE(mem_mask);
+ if (virDomainObjIsActive(vm))
+ vm->def->vcpus = vcpus;
virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);
NOTE: We'll audit regardless of ExitMonitor status here.
ACK in general, but the Audit stuff needs to be handled in the same
manner as other calls.
John
if (cgroup_vcpu)
virCgroupFree(&cgroup_vcpu);
@@ -4527,6 +4531,8 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
unsupported:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot change vcpu count of this domain"));
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto cleanup;
}
9:42 a.m.
New subject: [libvirt] [PATCHv2 07/14] Fix vmdef usage while in monitor in BlockStat* APIs
---
src/qemu/qemu_driver.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1275ba4..30c9017 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10114,6 +10114,7 @@ qemuDomainBlockStats(virDomainPtr dom,
virDomainObjPtr vm;
virDomainDiskDefPtr disk = NULL;
qemuDomainObjPrivatePtr priv;
+ char *diskAlias = NULL;
if (!*path) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@@ -10149,11 +10150,14 @@ qemuDomainBlockStats(virDomainPtr dom,
goto endjob;
}
+ if (VIR_STRDUP(diskAlias, disk->info.alias) < 0)
+ goto endjob;
+
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockStatsInfo(priv->mon,
- disk->info.alias,
+ diskAlias,
&stats->rd_req,
&stats->rd_bytes,
NULL,
@@ -10163,13 +10167,15 @@ qemuDomainBlockStats(virDomainPtr dom,
NULL,
NULL,
&stats->errs);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
cleanup:
qemuDomObjEndAPI(&vm);
+ VIR_FREE(diskAlias);
return ret;
}
@@ -10185,11 +10191,11 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
int idx;
int tmp, ret = -1;
virDomainObjPtr vm;
- virDomainDiskDefPtr disk = NULL;
qemuDomainObjPrivatePtr priv;
long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times;
long long wr_total_times, flush_req, flush_total_times, errs;
virTypedParameterPtr param;
+ char *diskAlias = NULL;
virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
@@ -10218,6 +10224,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
}
if (*nparams != 0) {
+ virDomainDiskDefPtr disk = NULL;
if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
virReportError(VIR_ERR_INVALID_ARG,
_("invalid path: %s"), path);
@@ -10231,6 +10238,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
disk->dst);
goto endjob;
}
+ if (VIR_STRDUP(diskAlias, disk->info.alias) < 0)
+ goto endjob;
}
priv = vm->privateData;
@@ -10241,12 +10250,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);
if (tmp == 0 || ret < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto endjob;
}
ret = qemuMonitorGetBlockStatsInfo(priv->mon,
- disk->info.alias,
+ diskAlias,
&rd_req,
&rd_bytes,
&rd_total_times,
@@ -10257,7 +10266,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
&flush_total_times,
&errs);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
if (ret < 0)
goto endjob;
@@ -10342,6 +10352,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
qemuDomainObjEndJob(driver, vm);
cleanup:
+ VIR_FREE(diskAlias);
qemuDomObjEndAPI(&vm);
return ret;
}
@@ -16853,7 +16864,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,
&info, supportMaxOptions);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (ret < 0)
goto endjob;
vm->def->disks[idx]->blkdeviotune = info;
--
2.0.4
4:30 p.m.
New subject: [libvirt] [PATCHv2 07/14] Fix vmdef usage while in monitor in BlockStat* APIs
<No commit message>?
Seems we're fixing two issues here
#1. The ExitMonitor issue
#2. Somehow the disk->info.alias becomes NULL and thus we're making a
local copy to avoid that.
On 01/07/2015 10:42 AM, Ján Tomko wrote:
---
src/qemu/qemu_driver.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1275ba4..30c9017 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10114,6 +10114,7 @@ qemuDomainBlockStats(virDomainPtr dom,
virDomainObjPtr vm;
virDomainDiskDefPtr disk = NULL;
qemuDomainObjPrivatePtr priv;
+ char *diskAlias = NULL;
if (!*path) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@@ -10149,11 +10150,14 @@ qemuDomainBlockStats(virDomainPtr dom,
goto endjob;
}
+ if (VIR_STRDUP(diskAlias, disk->info.alias) < 0)
+ goto endjob;
+
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockStatsInfo(priv->mon,
- disk->info.alias,
+ diskAlias,
&stats->rd_req,
&stats->rd_bytes,
NULL,
@@ -10163,13 +10167,15 @@ qemuDomainBlockStats(virDomainPtr dom,
NULL,
NULL,
&stats->errs);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
cleanup:
qemuDomObjEndAPI(&vm);
+ VIR_FREE(diskAlias);
return ret;
}
@@ -10185,11 +10191,11 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
int idx;
int tmp, ret = -1;
virDomainObjPtr vm;
- virDomainDiskDefPtr disk = NULL;
qemuDomainObjPrivatePtr priv;
long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times;
long long wr_total_times, flush_req, flush_total_times, errs;
virTypedParameterPtr param;
+ char *diskAlias = NULL;
virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
@@ -10218,6 +10224,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
}
if (*nparams != 0) {
+ virDomainDiskDefPtr disk = NULL;
if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
virReportError(VIR_ERR_INVALID_ARG,
_("invalid path: %s"), path);
@@ -10231,6 +10238,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
disk->dst);
goto endjob;
}
+ if (VIR_STRDUP(diskAlias, disk->info.alias) < 0)
+ goto endjob;
so diskAlias is only set if *nparams != 0, but...
}
priv = vm->privateData;
@@ -10241,12 +10250,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);
if (tmp == 0 || ret < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
Can tmp == 0 and ret == 0? Thus when go to endjob and eventually exit
we don't a failure? Does it matter? I suppose for consistency it does.
goto endjob;
}
ret = qemuMonitorGetBlockStatsInfo(priv->mon,
- disk->info.alias,
+ diskAlias,
...used here regardless of *nparams... Can we get here with diskAlias
== NULL?
&rd_req,
&rd_bytes,
&rd_total_times,
@@ -10257,7 +10266,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
&flush_total_times,
&errs);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
if (ret < 0)
goto endjob;
@@ -10342,6 +10352,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
qemuDomainObjEndJob(driver, vm);
cleanup:
+ VIR_FREE(diskAlias);
qemuDomObjEndAPI(&vm);
return ret;
}
@@ -16853,7 +16864,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,
&info, supportMaxOptions);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
Potentially leaving ret = 0 from the IoThrottle call... Should just set
ret = -1 (liek done in qemuDomainBlockStatsFlags above)
ACK with a commit message and the issues resolved.
John
if (ret < 0)
goto endjob;
vm->def->disks[idx]->blkdeviotune = info;
6:38 a.m.
New subject: [libvirt] [PATCHv2 07/14] Fix vmdef usage while in monitor in BlockStat* APIs
On 01/12/2015 11:30 PM, John Ferlan wrote:
<No commit message>?
Seems we're fixing two issues here
#1. The ExitMonitor issue
#2. Somehow the disk->info.alias becomes NULL and thus we're making a
local copy to avoid that.
On 01/07/2015 10:42 AM, Ján Tomko wrote:
> ---
> src/qemu/qemu_driver.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> @@ -10231,6 +10238,8 @@ qemuDomainBlockStatsFlags(virDomainPtr
dom,
> disk->dst);
> goto endjob;
> }
> + if (VIR_STRDUP(diskAlias, disk->info.alias) < 0)
> + goto endjob;
so diskAlias is only set if *nparams != 0, but...
> }
>
> priv = vm->privateData;
> @@ -10241,12 +10250,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
> ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);
>
> if (tmp == 0 || ret < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
Can tmp == 0 and ret == 0? Thus when go to endjob and eventually exit
we don't a failure? Does it matter? I suppose for consistency it does.
Just a few lines above *nparams gets assigned to tmp. So if no block stats
were requested, there's nothing to return back to the user and no need to fail.
> goto endjob;
> }
>
> ret = qemuMonitorGetBlockStatsInfo(priv->mon,
> - disk->info.alias,
> + diskAlias,
...used here regardless of *nparams... Can we get here with diskAlias
== NULL?
No, if tmp == 0 we jump to endjob.
> &rd_req,
> &rd_bytes,
> &rd_total_times,
> @@ -10257,7 +10266,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
> &flush_total_times,
> &errs);
>
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> if (ret < 0)
> goto endjob;
> @@ -10342,6 +10352,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
> qemuDomainObjEndJob(driver, vm);
>
> cleanup:
> + VIR_FREE(diskAlias);
> qemuDomObjEndAPI(&vm);
> return ret;
> }
> @@ -16853,7 +16864,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,
> &info, supportMaxOptions);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
Potentially leaving ret = 0 from the IoThrottle call... Should just set
ret = -1 (liek done in qemuDomainBlockStatsFlags above)
I've fixed that and wrote a commit message.
Jan
ACK with a commit message and the issues resolved.
John
> if (ret < 0)
> goto endjob;
> vm->def->disks[idx]->blkdeviotune = info;
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:42 a.m.
New subject: [libvirt] [PATCHv2 08/14] Fix vmdef usage while in monitor in qemu process
---
src/qemu/qemu_process.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4528b58..3d383ef 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2584,6 +2584,7 @@ qemuProcessInitPasswords(virConnectPtr conn,
qemuDomainObjPrivatePtr priv = vm->privateData;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
size_t i;
+ char *alias = NULL;
for (i = 0; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
@@ -2609,7 +2610,6 @@ qemuProcessInitPasswords(virConnectPtr conn,
for (i = 0; i < vm->def->ndisks; i++) {
char *secret;
size_t secretLen;
- const char *alias;
if (!vm->def->disks[i]->src->encryption ||
!virDomainDiskGetSource(vm->def->disks[i]))
@@ -2620,20 +2620,26 @@ qemuProcessInitPasswords(virConnectPtr conn,
&secret, &secretLen) < 0)
goto cleanup;
- alias = vm->def->disks[i]->info.alias;
+ VIR_FREE(alias);
+ if (VIR_STRDUP(alias, vm->def->disks[i]->info.alias) < 0)
+ goto cleanup;
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) {
VIR_FREE(secret);
goto cleanup;
}
ret = qemuMonitorSetDrivePassphrase(priv->mon, alias, secret);
VIR_FREE(secret);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
if (ret < 0)
goto cleanup;
}
}
cleanup:
+ VIR_FREE(alias);
virObjectUnref(cfg);
return ret;
}
@@ -4181,6 +4187,7 @@ int qemuProcessStart(virConnectPtr conn,
virCommandPtr cmd = NULL;
struct qemuProcessHookData hookData;
unsigned long cur_balloon;
+ unsigned int period = 0;
size_t i;
bool rawio_set = false;
char *nodeset = NULL;
@@ -4793,15 +4800,18 @@ int qemuProcessStart(virConnectPtr conn,
vm->def->mem.cur_balloon);
goto cleanup;
}
+ if (vm->def->memballoon && vm->def->memballoon->period)
+ period = vm->def->memballoon->period;
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
- if (vm->def->memballoon && vm->def->memballoon->period)
- qemuMonitorSetMemoryStatsPeriod(priv->mon,
vm->def->memballoon->period);
+ if (period)
+ qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) {
qemuDomainObjExitMonitor(driver, vm);
goto cleanup;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (!(flags & VIR_QEMU_PROCESS_START_PAUSED)) {
VIR_DEBUG("Starting domain CPUs");
--
2.0.4
4:37 p.m.
New subject: [libvirt] [PATCHv2 08/14] Fix vmdef usage while in monitor in qemu process
<no commit message>?
Since it seems there's a second bugfix here (eg, the alias setting),
perhaps one should be added.
On 01/07/2015 10:42 AM, Ján Tomko wrote:
---
src/qemu/qemu_process.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4528b58..3d383ef 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2584,6 +2584,7 @@ qemuProcessInitPasswords(virConnectPtr conn,
qemuDomainObjPrivatePtr priv = vm->privateData;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
size_t i;
+ char *alias = NULL;
for (i = 0; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
@@ -2609,7 +2610,6 @@ qemuProcessInitPasswords(virConnectPtr conn,
for (i = 0; i < vm->def->ndisks; i++) {
char *secret;
size_t secretLen;
- const char *alias;
if (!vm->def->disks[i]->src->encryption ||
!virDomainDiskGetSource(vm->def->disks[i]))
@@ -2620,20 +2620,26 @@ qemuProcessInitPasswords(virConnectPtr conn,
&secret, &secretLen) < 0)
goto cleanup;
- alias = vm->def->disks[i]->info.alias;
+ VIR_FREE(alias);
+ if (VIR_STRDUP(alias, vm->def->disks[i]->info.alias) < 0)
+ goto cleanup;
This will leak secret (my coverity checker found this)
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob)
< 0) {
VIR_FREE(secret);
goto cleanup;
}
ret = qemuMonitorSetDrivePassphrase(priv->mon, alias, secret);
VIR_FREE(secret);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
superfluous considering the following statement
+ }
if (ret < 0)
goto cleanup;
}
}
cleanup:
+ VIR_FREE(alias);
virObjectUnref(cfg);
return ret;
}
@@ -4181,6 +4187,7 @@ int qemuProcessStart(virConnectPtr conn,
virCommandPtr cmd = NULL;
struct qemuProcessHookData hookData;
unsigned long cur_balloon;
+ unsigned int period = 0;
size_t i;
bool rawio_set = false;
char *nodeset = NULL;
@@ -4793,15 +4800,18 @@ int qemuProcessStart(virConnectPtr conn,
vm->def->mem.cur_balloon);
goto cleanup;
}
+ if (vm->def->memballoon && vm->def->memballoon->period)
+ period = vm->def->memballoon->period;
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
- if (vm->def->memballoon && vm->def->memballoon->period)
- qemuMonitorSetMemoryStatsPeriod(priv->mon,
vm->def->memballoon->period);
+ if (period)
+ qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) {
qemuDomainObjExitMonitor(driver, vm);
Hmm no status check on this one? Although fixed in 11/14 it seems
ACK with adjustments
John
goto cleanup;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (!(flags & VIR_QEMU_PROCESS_START_PAUSED)) {
VIR_DEBUG("Starting domain CPUs");
9:42 a.m.
New subject: [libvirt] [PATCHv2 09/14] Exit early after domain crash in monitor on device hotplug
Error out if the domain has disappeared in the meantime.
This prevents writing the persistent definition as the domain
status and calling qemuDomainRemoveDevice on devices
that already have been dealt with in qemuProcessStop.
---
src/qemu/qemu_domain.c | 9 +++++----
src/qemu/qemu_driver.c | 4 ++--
src/qemu/qemu_hotplug.c | 38 +++++++++++++++++++-------------------
3 files changed, 26 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c942b02..82d0c91 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2748,17 +2748,18 @@ qemuDomainUpdateDeviceList(virQEMUDriverPtr driver,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
char **aliases;
+ int rc;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
return 0;
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
- if (qemuMonitorGetDeviceAliases(priv->mon, &aliases) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ rc = qemuMonitorGetDeviceAliases(priv->mon, &aliases);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
+ if (rc < 0)
return -1;
- }
- qemuDomainObjExitMonitor(driver, vm);
virStringFreeList(priv->qemuDevices);
priv->qemuDevices = aliases;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 30c9017..1781ea9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6982,7 +6982,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
}
if (ret == 0)
- qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
+ ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
return ret;
}
@@ -7058,7 +7058,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
}
if (ret == 0)
- qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
+ ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
return ret;
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a4e4d6b..6b108bf 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -193,7 +193,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (ret < 0)
goto error;
@@ -236,7 +237,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
driveAlias,
sourcestr,
format);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
}
virDomainAuditDisk(vm, disk->src, newsrc, "update", ret >= 0);
@@ -275,7 +277,8 @@ qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
table = qemuMonitorGetBlockInfo(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
}
if (!table)
@@ -1017,7 +1020,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuMonitorAddNetdev(priv->mon, netstr,
tapfd, tapfdName, tapfdSize,
vhostfd, vhostfdName, vhostfdSize) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
virDomainAuditNet(vm, NULL, net, "attach", false);
goto cleanup;
}
@@ -1025,24 +1028,19 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuMonitorAddHostNetwork(priv->mon, netstr,
tapfd, tapfdName, tapfdSize,
vhostfd, vhostfdName, vhostfdSize) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
virDomainAuditNet(vm, NULL, net, "attach", false);
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
for (i = 0; i < tapfdSize; i++)
VIR_FORCE_CLOSE(tapfd[i]);
for (i = 0; i < vhostfdSize; i++)
VIR_FORCE_CLOSE(vhostfd[i]);
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
- goto cleanup;
- }
-
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (!(nicstr = qemuBuildNicDevStr(vm->def, net, vlan, 0,
vhostfdSize, priv->qemuCaps)))
@@ -1055,7 +1053,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
qemuDomainObjEnterMonitor(driver, vm);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
virDomainAuditNet(vm, NULL, net, "attach", false);
goto try_remove;
}
@@ -1063,14 +1061,15 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
guestAddr = net->info.addr.pci;
if (qemuMonitorAddPCINetwork(priv->mon, nicstr,
&guestAddr) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
virDomainAuditNet(vm, NULL, net, "attach", false);
goto try_remove;
}
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
memcpy(&net->info.addr.pci, &guestAddr, sizeof(guestAddr));
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
/* set link state */
if (net->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) {
@@ -1082,7 +1081,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
if (qemuMonitorSetLink(priv->mon, net->info.alias,
VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
virDomainAuditNet(vm, NULL, net, "attach", false);
goto try_remove;
}
@@ -1091,7 +1090,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
_("setting of link state not supported: Link is
up"));
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
}
/* link set to down */
}
@@ -1165,7 +1165,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
VIR_WARN("Failed to remove network backend for netdev %s",
netdev_name);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
VIR_FREE(netdev_name);
} else {
VIR_WARN("Unable to remove network backend");
@@ -1178,7 +1178,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0)
VIR_WARN("Failed to remove network backend for vlan %d, net %s",
vlan, hostnet_name);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
VIR_FREE(hostnet_name);
}
goto cleanup;
--
2.0.4
5:54 p.m.
New subject: [libvirt] [PATCHv2 09/14] Exit early after domain crash in monitor on device hotplug
On 01/07/2015 10:42 AM, Ján Tomko wrote:
Error out if the domain has disappeared in the meantime.
This prevents writing the persistent definition as the domain
status and calling qemuDomainRemoveDevice on devices
that already have been dealt with in qemuProcessStop.
---
src/qemu/qemu_domain.c | 9 +++++----
src/qemu/qemu_driver.c | 4 ++--
src/qemu/qemu_hotplug.c | 38 +++++++++++++++++++-------------------
3 files changed, 26 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c942b02..82d0c91 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2748,17 +2748,18 @@ qemuDomainUpdateDeviceList(virQEMUDriverPtr driver,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
char **aliases;
+ int rc;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
return 0;
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
- if (qemuMonitorGetDeviceAliases(priv->mon, &aliases) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ rc = qemuMonitorGetDeviceAliases(priv->mon, &aliases);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
+ if (rc < 0)
return -1;
- }
- qemuDomainObjExitMonitor(driver, vm);
virStringFreeList(priv->qemuDevices);
priv->qemuDevices = aliases;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 30c9017..1781ea9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6982,7 +6982,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
}
if (ret == 0)
- qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
+ ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
return ret;
}
@@ -7058,7 +7058,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
}
if (ret == 0)
- qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
+ ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
return ret;
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a4e4d6b..6b108bf 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -193,7 +193,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if "ret == 0" we get to cleanup... with ejected media, no Audit and the
rest of this not done...
if (ret < 0)
goto error;
@@ -236,7 +237,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
driveAlias,
sourcestr,
format);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
Same here - get to cleanup with ret == 0 and of course no audit.
}
virDomainAuditDisk(vm, disk->src, newsrc, "update", ret >= 0);
@@ -275,7 +277,8 @@ qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
table = qemuMonitorGetBlockInfo(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
}
if (!table)
@@ -1017,7 +1020,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuMonitorAddNetdev(priv->mon, netstr,
tapfd, tapfdName, tapfdSize,
vhostfd, vhostfdName, vhostfdSize) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
Here we are with the audit thing again
virDomainAuditNet(vm, NULL, net, "attach",
false);
goto cleanup;
}
@@ -1025,24 +1028,19 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuMonitorAddHostNetwork(priv->mon, netstr,
tapfd, tapfdName, tapfdSize,
vhostfd, vhostfdName, vhostfdSize) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
again
virDomainAuditNet(vm, NULL, net, "attach",
false);
goto cleanup;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
for (i = 0; i < tapfdSize; i++)
VIR_FORCE_CLOSE(tapfd[i]);
for (i = 0; i < vhostfdSize; i++)
VIR_FORCE_CLOSE(vhostfd[i]);
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
- goto cleanup;
- }
-
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (!(nicstr = qemuBuildNicDevStr(vm->def, net, vlan, 0,
vhostfdSize, priv->qemuCaps)))
@@ -1055,7 +1053,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
qemuDomainObjEnterMonitor(driver, vm);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
Audit ...
virDomainAuditNet(vm, NULL, net, "attach",
false);
goto try_remove;
}
@@ -1063,14 +1061,15 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
guestAddr = net->info.addr.pci;
if (qemuMonitorAddPCINetwork(priv->mon, nicstr,
&guestAddr) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
Audit...
virDomainAuditNet(vm, NULL, net, "attach",
false);
goto try_remove;
}
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
memcpy(&net->info.addr.pci, &guestAddr, sizeof(guestAddr));
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
Audit - after the following if is only necessary if we are successful,
so I guess OK to not audit here; however, up through this point we could
have succeeded only to see the guest die...
/* set link state */
if (net->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) {
@@ -1082,7 +1081,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
if (qemuMonitorSetLink(priv->mon, net->info.alias,
VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
Audit...
virDomainAuditNet(vm, NULL, net,
"attach", false);
goto try_remove;
}
@@ -1091,7 +1090,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
_("setting of link state not supported: Link is
up"));
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
Audit...
}
/* link set to down */
}
FWIW: Successful Audit call is right here....
Not Auditing beyond here here seems fine and it's all error/exit path
anyway.
ACK once we clear up what the right thing to do with audit and of course
the place or two where we've returned without setting ret = -1 when
ExitMonitor fails.
John
@@ -1165,7 +1165,7 @@ int qemuDomainAttachNetDevice(virConnectPtr
conn,
if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
VIR_WARN("Failed to remove network backend for netdev %s",
netdev_name);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
VIR_FREE(netdev_name);
} else {
VIR_WARN("Unable to remove network backend");
@@ -1178,7 +1178,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0)
VIR_WARN("Failed to remove network backend for vlan %d, net %s",
vlan, hostnet_name);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
VIR_FREE(hostnet_name);
}
goto cleanup;
6:38 a.m.
New subject: [libvirt] [PATCHv2 09/14] Exit early after domain crash in monitor on device hotplug
On 01/13/2015 12:54 AM, John Ferlan wrote:
On 01/07/2015 10:42 AM, Ján Tomko wrote:
> Error out if the domain has disappeared in the meantime.
>
> This prevents writing the persistent definition as the domain
> status and calling qemuDomainRemoveDevice on devices
> that already have been dealt with in qemuProcessStop.
> ---
> src/qemu/qemu_domain.c | 9 +++++----
> src/qemu/qemu_driver.c | 4 ++--
> src/qemu/qemu_hotplug.c | 38 +++++++++++++++++++-------------------
> 3 files changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index a4e4d6b..6b108bf 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -193,7 +193,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
if "ret == 0" we get to cleanup... with ejected media, no Audit and the
rest of this not done...
>
I've added ret = -1 in front of that. Both audit and qemuDomainPrepareDisk on
the error path would need access to the disk that's now freed.
Jan
9:42 a.m.
New subject: [libvirt] [PATCHv2 10/14] Exit early after domain crash in monitor on migration
---
src/qemu/qemu_migration.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 77e0b35..31494c8 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1663,7 +1663,8 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
qemuDomainObjExitMonitor(driver, vm);
goto cleanup;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
}
priv->nbdPort = port;
@@ -1860,7 +1861,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
}
-static void
+static int
qemuMigrationStopNBDServer(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuMigrationCookiePtr mig)
@@ -1868,22 +1869,23 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
if (!mig->nbd)
- return;
+ return 0;
if (qemuDomainObjEnterMonitorAsync(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
- return;
+ return -1;
if (qemuMonitorNBDServerStop(priv->mon) < 0)
VIR_WARN("Unable to stop NBD server");
- qemuDomainObjExitMonitor(driver, vm);
-
virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort);
priv->nbdPort = 0;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
+ return 0;
}
-static void
+static int
qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
virQEMUDriverPtr driver,
virDomainObjPtr vm)
@@ -1891,6 +1893,7 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
qemuDomainObjPrivatePtr priv = vm->privateData;
size_t i;
char *diskAlias = NULL;
+ int ret = 0;
VIR_DEBUG("mig=%p nbdPort=%d", mig->nbd, priv->nbdPort);
@@ -1914,12 +1917,15 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0,
BLOCK_JOB_ABORT, true) < 0)
VIR_WARN("Unable to stop block job on %s", diskAlias);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
}
cleanup:
VIR_FREE(diskAlias);
- return;
+ return ret;
}
/* Validate whether the domain is safe to migrate. If vm is NULL,
@@ -2125,7 +2131,8 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
state);
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
return ret;
}
@@ -2164,7 +2171,8 @@ qemuMigrationSetAutoConverge(virQEMUDriverPtr driver,
state);
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
return ret;
}
@@ -2210,7 +2218,8 @@ qemuMigrationSetPinAll(virQEMUDriverPtr driver,
state);
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
return ret;
}
@@ -2476,7 +2485,8 @@ qemuDomainMigrateGraphicsRelocate(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
ret = qemuMonitorGraphicsRelocate(priv->mon, type, listenAddress,
port, tlsPort, tlsSubject);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
}
cleanup:
@@ -4000,7 +4010,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
/* cancel any outstanding NBD jobs */
if (mig)
- qemuMigrationCancelDriveMirror(mig, driver, vm);
+ ignore_value(qemuMigrationCancelDriveMirror(mig, driver, vm));
if (spec->fwdType != MIGRATION_FWD_DIRECT) {
if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0)
@@ -5142,7 +5152,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
VIR_WARN("unable to provide network data for relocation");
}
- qemuMigrationStopNBDServer(driver, vm, mig);
+ if (qemuMigrationStopNBDServer(driver, vm, mig) < 0)
+ goto endjob;
if (flags & VIR_MIGRATE_PERSIST_DEST) {
virDomainDefPtr vmdef;
--
2.0.4
6:44 p.m.
New subject: [libvirt] [PATCHv2 10/14] Exit early after domain crash in monitor on migration
<no commit message>
On 01/07/2015 10:42 AM, Ján Tomko wrote:
---
src/qemu/qemu_migration.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 77e0b35..31494c8 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1663,7 +1663,8 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
qemuDomainObjExitMonitor(driver, vm);
Missing the error check here (yes, it's in 11/14)
goto cleanup;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
}
priv->nbdPort = port;
@@ -1860,7 +1861,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
}
-static void
+static int
qemuMigrationStopNBDServer(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuMigrationCookiePtr mig)
@@ -1868,22 +1869,23 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
if (!mig->nbd)
- return;
+ return 0;
if (qemuDomainObjEnterMonitorAsync(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
- return;
+ return -1;
if (qemuMonitorNBDServerStop(priv->mon) < 0)
VIR_WARN("Unable to stop NBD server");
- qemuDomainObjExitMonitor(driver, vm);
-
Why not ret = qemuDomainObjExitMonitor(), then return ret later on...
Holding the lock while virPortAllocatorRelease executes seems unnecessary...
virPortAllocatorRelease(driver->migrationPorts,
priv->nbdPort);
priv->nbdPort = 0;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
+ return 0;
}
-static void
+static int
qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
virQEMUDriverPtr driver,
virDomainObjPtr vm)
@@ -1891,6 +1893,7 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
qemuDomainObjPrivatePtr priv = vm->privateData;
size_t i;
char *diskAlias = NULL;
+ int ret = 0;
VIR_DEBUG("mig=%p nbdPort=%d", mig->nbd, priv->nbdPort);
@@ -1914,12 +1917,15 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0,
BLOCK_JOB_ABORT, true) < 0)
VIR_WARN("Unable to stop block job on %s", diskAlias);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
}
cleanup:
VIR_FREE(diskAlias);
- return;
+ return ret;
}
/* Validate whether the domain is safe to migrate. If vm is NULL,
@@ -2125,7 +2131,8 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
state);
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
or ret = qemuDomainObjExitMonitor();
return ret;
(your choice here and the next few)
The rest seems OK... Might be nice to mention in a commit message why
failure to cancel drive mirror is ignorable while failure to stop nbd
server is not.
ACK, but it might be nice to pull in part of 11/14 since you're changing
ExitMonitor checks anyway
John
return ret;
}
@@ -2164,7 +2171,8 @@ qemuMigrationSetAutoConverge(virQEMUDriverPtr driver,
state);
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
return ret;
}
@@ -2210,7 +2218,8 @@ qemuMigrationSetPinAll(virQEMUDriverPtr driver,
state);
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
return ret;
}
@@ -2476,7 +2485,8 @@ qemuDomainMigrateGraphicsRelocate(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
ret = qemuMonitorGraphicsRelocate(priv->mon, type, listenAddress,
port, tlsPort, tlsSubject);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
}
cleanup:
@@ -4000,7 +4010,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
/* cancel any outstanding NBD jobs */
if (mig)
- qemuMigrationCancelDriveMirror(mig, driver, vm);
+ ignore_value(qemuMigrationCancelDriveMirror(mig, driver, vm));
if (spec->fwdType != MIGRATION_FWD_DIRECT) {
if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0)
@@ -5142,7 +5152,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
VIR_WARN("unable to provide network data for
relocation");
}
- qemuMigrationStopNBDServer(driver, vm, mig);
+ if (qemuMigrationStopNBDServer(driver, vm, mig) < 0)
+ goto endjob;
if (flags & VIR_MIGRATE_PERSIST_DEST) {
virDomainDefPtr vmdef;
9:42 a.m.
New subject: [libvirt] [PATCHv2 11/14] Exit early after domain crash in monitor in qemu_process
---
src/qemu/qemu_process.c | 102 +++++++++++++++++++++++++-----------------------
1 file changed, 53 insertions(+), 49 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3d383ef..e6c5bc6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -571,7 +571,7 @@ qemuProcessFakeReboot(void *opaque)
virObjectEventPtr event = NULL;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virDomainRunningReason reason = VIR_DOMAIN_RUNNING_BOOTED;
- int ret = -1;
+ int ret = -1, rc;
VIR_DEBUG("vm=%p", vm);
virObjectLock(vm);
@@ -585,17 +585,13 @@ qemuProcessFakeReboot(void *opaque)
}
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorSystemReset(priv->mon) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ rc = qemuMonitorSystemReset(priv->mon);
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto endjob;
- }
- qemuDomainObjExitMonitor(driver, vm);
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
+ if (rc < 0)
goto endjob;
- }
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_CRASHED)
reason = VIR_DOMAIN_RUNNING_CRASHED;
@@ -1662,7 +1658,8 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int
asyncJob,
if (ret == 0 &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON))
ret = virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
error:
@@ -2118,7 +2115,8 @@ qemuProcessReconnectRefreshChannelVirtioState(virQEMUDriverPtr
driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetChardevInfo(priv->mon, &info);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (ret < 0)
goto cleanup;
@@ -2171,7 +2169,8 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
ret = qemuMonitorGetChardevInfo(priv->mon, &info);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
VIR_DEBUG("qemuMonitorGetChardevInfo returned %i", ret);
if (ret == 0) {
@@ -2264,18 +2263,19 @@ qemuProcessDetectVcpuPIDs(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
+ ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
/* failure to get the VCPU<-> PID mapping or to execute the query
* command will not be treated fatal as some versions of qemu don't
* support this command */
- if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (ncpupids <= 0) {
virResetLastError();
priv->nvcpupids = 0;
priv->vcpupids = NULL;
return 0;
}
- qemuDomainObjExitMonitor(driver, vm);
if (ncpupids != vm->def->vcpus) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2310,7 +2310,8 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
niothreads = qemuMonitorGetIOThreads(priv->mon, &iothreads);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (niothreads < 0)
goto cleanup;
@@ -3026,11 +3027,13 @@ qemuProcessInitPCIAddresses(virQEMUDriverPtr driver,
return -1;
naddrs = qemuMonitorGetAllPCIAddresses(priv->mon,
&addrs);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (naddrs > 0)
ret = qemuProcessDetectPCIAddresses(vm, addrs, naddrs);
+ cleanup:
VIR_FREE(addrs);
return ret;
@@ -3180,7 +3183,8 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm,
goto release;
ret = qemuMonitorStartCPUs(priv->mon, conn);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto release;
if (ret < 0)
goto release;
@@ -3213,7 +3217,8 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver,
goto cleanup;
ret = qemuMonitorStopCPUs(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (ret < 0)
goto cleanup;
@@ -3282,9 +3287,10 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr
vm)
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetStatus(priv->mon, &running, &reason);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
- if (ret < 0 || !virDomainObjIsActive(vm))
+ if (ret < 0)
return -1;
state = virDomainObjGetState(vm, NULL);
@@ -3396,7 +3402,8 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver,
vm->def->name);
qemuDomainObjEnterMonitor(driver, vm);
ignore_value(qemuMonitorMigrateCancel(priv->mon));
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
/* resume the domain but only if it was paused as a result of
* migration */
if (state == VIR_DOMAIN_PAUSED &&
@@ -3465,7 +3472,8 @@ qemuProcessRecoverJob(virQEMUDriverPtr driver,
case QEMU_ASYNC_JOB_SNAPSHOT:
qemuDomainObjEnterMonitor(driver, vm);
ignore_value(qemuMonitorMigrateCancel(priv->mon));
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
/* resume the domain but only if it was paused as a result of
* running a migration-to-file operation. Although we are
* recovering an async job, this function is run at startup
@@ -3990,7 +3998,8 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return false;
rc = qemuMonitorGetGuestCPU(priv->mon, arch, &guestcpu);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return false;
if (rc < 0) {
if (rc == -2)
@@ -4777,12 +4786,10 @@ int qemuProcessStart(virConnectPtr conn,
VIR_DEBUG("Setting network link states");
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
- if (qemuProcessSetLinkStates(vm) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuProcessSetLinkStates(vm) < 0)
+ goto exit_monitor;
+ if (qemuDomainObjExitMonitor(driver, vm))
goto cleanup;
- }
-
- qemuDomainObjExitMonitor(driver, vm);
VIR_DEBUG("Fetching list of active devices");
if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0)
@@ -4806,10 +4813,8 @@ int qemuProcessStart(virConnectPtr conn,
goto cleanup;
if (period)
qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
- if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
- }
+ if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0)
+ goto exit_monitor;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
@@ -4882,6 +4887,10 @@ int qemuProcessStart(virConnectPtr conn,
virObjectUnref(caps);
return -1;
+
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ goto cleanup;
}
@@ -5388,21 +5397,13 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
VIR_DEBUG("Getting initial memory amount");
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorGetBalloonInfo(priv->mon, &vm->def->mem.cur_balloon) <
0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto error;
- }
- if (qemuMonitorGetStatus(priv->mon, &running, &reason) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto error;
- }
- if (qemuMonitorGetVirtType(priv->mon, &vm->def->virtType) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto error;
- }
- qemuDomainObjExitMonitor(driver, vm);
-
- if (!virDomainObjIsActive(vm))
+ if (qemuMonitorGetBalloonInfo(priv->mon, &vm->def->mem.cur_balloon) <
0)
+ goto exit_monitor;
+ if (qemuMonitorGetStatus(priv->mon, &running, &reason) < 0)
+ goto exit_monitor;
+ if (qemuMonitorGetVirtType(priv->mon, &vm->def->virtType) < 0)
+ goto exit_monitor;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto error;
if (running) {
@@ -5412,7 +5413,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorSetMemoryStatsPeriod(priv->mon,
vm->def->memballoon->period);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto error;
}
} else {
virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason);
@@ -5447,6 +5449,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
return 0;
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
error:
/* We jump here if we failed to attach to the VM for any reason.
* Leave the domain running, but pretend we never attempted to
--
2.0.4
7:05 p.m.
New subject: [libvirt] [PATCHv2 11/14] Exit early after domain crash in monitor in qemu_process
<no commit message>
On 01/07/2015 10:42 AM, Ján Tomko wrote:
---
src/qemu/qemu_process.c | 102 +++++++++++++++++++++++++-----------------------
1 file changed, 53 insertions(+), 49 deletions(-)
Hmm.. miscounted in a couple of comments in previous changes where I
reference 11/14 - guess that's 12/14 <sigh> it's late.
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3d383ef..e6c5bc6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -571,7 +571,7 @@ qemuProcessFakeReboot(void *opaque)
virObjectEventPtr event = NULL;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virDomainRunningReason reason = VIR_DOMAIN_RUNNING_BOOTED;
- int ret = -1;
+ int ret = -1, rc;
VIR_DEBUG("vm=%p", vm);
virObjectLock(vm);
@@ -585,17 +585,13 @@ qemuProcessFakeReboot(void *opaque)
}
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorSystemReset(priv->mon) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ rc = qemuMonitorSystemReset(priv->mon);
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto endjob;
- }
- qemuDomainObjExitMonitor(driver, vm);
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
+ if (rc < 0)
goto endjob;
- }
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_CRASHED)
reason = VIR_DOMAIN_RUNNING_CRASHED;
@@ -1662,7 +1658,8 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int
asyncJob,
if (ret == 0 &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON))
ret = virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
error:
@@ -2118,7 +2115,8 @@ qemuProcessReconnectRefreshChannelVirtioState(virQEMUDriverPtr
driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetChardevInfo(priv->mon, &info);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
^^
But if ret == 0, then we may get erroneous report of success.
if (ret < 0)
goto cleanup;
@@ -2171,7 +2169,8 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
ret = qemuMonitorGetChardevInfo(priv->mon, &info);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
If ret == 0, then could we get erroneous report of success?
VIR_DEBUG("qemuMonitorGetChardevInfo returned %i", ret);
NIT: This could move up a couple lines right after the call...
if (ret == 0) {
@@ -2264,18 +2263,19 @@ qemuProcessDetectVcpuPIDs(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
+ ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
/* failure to get the VCPU<-> PID mapping or to execute the query
* command will not be treated fatal as some versions of qemu don't
* support this command */
- if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (ncpupids <= 0) {
virResetLastError();
priv->nvcpupids = 0;
priv->vcpupids = NULL;
return 0;
}
- qemuDomainObjExitMonitor(driver, vm);
if (ncpupids != vm->def->vcpus) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2310,7 +2310,8 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
niothreads = qemuMonitorGetIOThreads(priv->mon, &iothreads);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (niothreads < 0)
goto cleanup;
@@ -3026,11 +3027,13 @@ qemuProcessInitPCIAddresses(virQEMUDriverPtr driver,
return -1;
naddrs = qemuMonitorGetAllPCIAddresses(priv->mon,
&addrs);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (naddrs > 0)
ret = qemuProcessDetectPCIAddresses(vm, addrs, naddrs);
+ cleanup:
VIR_FREE(addrs);
return ret;
@@ -3180,7 +3183,8 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm,
goto release;
ret = qemuMonitorStartCPUs(priv->mon, conn);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto release;
if ret == 0, then we spew that nasty message but return success.
if (ret < 0)
goto release;
@@ -3213,7 +3217,8 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver,
goto cleanup;
ret = qemuMonitorStopCPUs(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
If ret == 0, then we return success which is wrong.
The rest seems reasonable
ACK with changes
John
if (ret < 0)
goto cleanup;
@@ -3282,9 +3287,10 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr
vm)
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetStatus(priv->mon, &running, &reason);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
- if (ret < 0 || !virDomainObjIsActive(vm))
+ if (ret < 0)
return -1;
state = virDomainObjGetState(vm, NULL);
@@ -3396,7 +3402,8 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver,
vm->def->name);
qemuDomainObjEnterMonitor(driver, vm);
ignore_value(qemuMonitorMigrateCancel(priv->mon));
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
/* resume the domain but only if it was paused as a result of
* migration */
if (state == VIR_DOMAIN_PAUSED &&
@@ -3465,7 +3472,8 @@ qemuProcessRecoverJob(virQEMUDriverPtr driver,
case QEMU_ASYNC_JOB_SNAPSHOT:
qemuDomainObjEnterMonitor(driver, vm);
ignore_value(qemuMonitorMigrateCancel(priv->mon));
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
/* resume the domain but only if it was paused as a result of
* running a migration-to-file operation. Although we are
* recovering an async job, this function is run at startup
@@ -3990,7 +3998,8 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return false;
rc = qemuMonitorGetGuestCPU(priv->mon, arch, &guestcpu);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return false;
if (rc < 0) {
if (rc == -2)
@@ -4777,12 +4786,10 @@ int qemuProcessStart(virConnectPtr conn,
VIR_DEBUG("Setting network link states");
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
- if (qemuProcessSetLinkStates(vm) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuProcessSetLinkStates(vm) < 0)
+ goto exit_monitor;
+ if (qemuDomainObjExitMonitor(driver, vm))
goto cleanup;
- }
-
- qemuDomainObjExitMonitor(driver, vm);
VIR_DEBUG("Fetching list of active devices");
if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0)
@@ -4806,10 +4813,8 @@ int qemuProcessStart(virConnectPtr conn,
goto cleanup;
if (period)
qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
- if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
- }
+ if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0)
+ goto exit_monitor;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
@@ -4882,6 +4887,10 @@ int qemuProcessStart(virConnectPtr conn,
virObjectUnref(caps);
return -1;
+
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ goto cleanup;
}
@@ -5388,21 +5397,13 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
VIR_DEBUG("Getting initial memory amount");
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorGetBalloonInfo(priv->mon, &vm->def->mem.cur_balloon)
< 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto error;
- }
- if (qemuMonitorGetStatus(priv->mon, &running, &reason) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto error;
- }
- if (qemuMonitorGetVirtType(priv->mon, &vm->def->virtType) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto error;
- }
- qemuDomainObjExitMonitor(driver, vm);
-
- if (!virDomainObjIsActive(vm))
+ if (qemuMonitorGetBalloonInfo(priv->mon, &vm->def->mem.cur_balloon)
< 0)
+ goto exit_monitor;
+ if (qemuMonitorGetStatus(priv->mon, &running, &reason) < 0)
+ goto exit_monitor;
+ if (qemuMonitorGetVirtType(priv->mon, &vm->def->virtType) < 0)
+ goto exit_monitor;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto error;
if (running) {
@@ -5412,7 +5413,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorSetMemoryStatsPeriod(priv->mon,
vm->def->memballoon->period);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto error;
}
} else {
virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason);
@@ -5447,6 +5449,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
return 0;
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
error:
/* We jump here if we failed to attach to the VM for any reason.
* Leave the domain running, but pretend we never attempted to
9:42 a.m.
New subject: [libvirt] [PATCHv2 12/14] Exit early after domain crash in monitor in qemu_driver
---
src/qemu/qemu_driver.c | 131 +++++++++++++++++++++++++++++-----------------
src/qemu/qemu_migration.c | 98 +++++++++++++++-------------------
2 files changed, 125 insertions(+), 104 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1781ea9..55d6fb3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1978,7 +1978,8 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int
flags)
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemPowerdown(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
}
endjob:
@@ -2073,7 +2074,8 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
} else {
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemPowerdown(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
if (ret == 0)
qemuDomainSetFakeReboot(driver, vm, isReboot);
@@ -2116,7 +2118,8 @@ qemuDomainReset(virDomainPtr dom, unsigned int flags)
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemReset(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
priv->fakeReboot = false;
@@ -2333,7 +2336,8 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long
newmem,
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetBalloon(priv->mon, newmem);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
virDomainAuditMemory(vm, vm->def->mem.cur_balloon, newmem,
"update",
r == 1);
if (r < 0)
@@ -2415,7 +2419,8 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (r < 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("unable to set balloon driver collection
period"));
@@ -2479,7 +2484,8 @@ static int qemuDomainInjectNMI(virDomainPtr domain, unsigned int
flags)
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorInjectNMI(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -2541,7 +2547,8 @@ static int qemuDomainSendKey(virDomainPtr domain,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -2596,7 +2603,10 @@ static int qemuDomainGetInfo(virDomainPtr dom,
} else {
qemuDomainObjEnterMonitor(driver, vm);
err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ qemuDomainObjEndJob(driver, vm);
+ goto cleanup;
+ }
}
qemuDomainObjEndJob(driver, vm);
@@ -3492,7 +3502,7 @@ static int qemuDumpToFd(virQEMUDriverPtr driver, virDomainObjPtr
vm,
ret = qemuMonitorDumpToFd(priv->mon, fd, dumpformat);
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
return ret;
}
@@ -3691,7 +3701,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom,
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemReset(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
if (resume && virDomainObjIsActive(vm)) {
@@ -3788,10 +3798,11 @@ qemuDomainScreenshot(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorScreendump(priv->mon, tmp) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto endjob;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (VIR_CLOSE(tmp_fd) < 0) {
virReportSystemError(errno, _("unable to close %s"), tmp);
@@ -4216,7 +4227,8 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &guestFilter);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (ret < 0)
goto endjob;
@@ -6227,7 +6239,8 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ err = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -10083,10 +10096,11 @@ qemuDomainBlockResize(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorBlockResize(priv->mon, device, size) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto endjob;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
ret = 0;
@@ -10740,7 +10754,8 @@ qemuDomainMemoryStats(virDomainPtr dom,
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
if (ret >= 0 && ret < nr_stats) {
long rss;
@@ -10874,16 +10889,17 @@ qemuDomainMemoryPeek(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
if (flags == VIR_MEMORY_VIRTUAL) {
if (qemuMonitorSaveVirtualMemory(priv->mon, offset, size, tmp) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto endjob;
}
} else {
if (qemuMonitorSavePhysicalMemory(priv->mon, offset, size, tmp) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto endjob;
}
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
/* Read the memory file into buffer. */
if (saferead(fd, buffer, size) == (ssize_t) -1) {
@@ -11121,7 +11137,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
ret = qemuMonitorGetBlockExtent(priv->mon,
disk->info.alias,
&src->allocation);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
}
if (ret == 0) {
@@ -12361,7 +12378,8 @@ static int qemuDomainAbortJob(virDomainPtr dom)
qemuDomainObjAbortAsyncJob(vm);
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorMigrateCancel(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -12404,7 +12422,8 @@ qemuDomainMigrateSetMaxDowntime(virDomainPtr dom,
VIR_DEBUG("Setting migration downtime to %llums", downtime);
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSetMigrationDowntime(priv->mon, downtime);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -12457,7 +12476,8 @@ qemuDomainMigrateGetCompressionCache(virDomainPtr dom,
ret = qemuMonitorGetMigrationCacheSize(priv->mon, cacheSize);
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -12511,7 +12531,8 @@ qemuDomainMigrateSetCompressionCache(virDomainPtr dom,
ret = qemuMonitorSetMigrationCacheSize(priv->mon, cacheSize);
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -12561,7 +12582,8 @@ qemuDomainMigrateSetMaxSpeed(virDomainPtr dom,
VIR_DEBUG("Setting migration bandwidth to %luMbs", bandwidth);
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSetMigrationSpeed(priv->mon, bandwidth);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
if (ret == 0)
priv->migMaxBandwidth = bandwidth;
@@ -15006,7 +15028,10 @@ static int qemuDomainQemuMonitorCommand(virDomainPtr domain,
const char *cmd,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorArbitraryCommand(priv->mon, cmd, result, hmp);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto endjob;
+ }
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -15328,14 +15353,10 @@ qemuDomainBlockPivot(virConnectPtr conn,
if (!disk->mirrorState) {
qemuDomainObjEnterMonitor(driver, vm);
rc = qemuMonitorBlockJobInfo(priv->mon, device, &info, NULL);
- qemuDomainObjExitMonitor(driver, vm);
- if (rc < 0)
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("domain is not running"));
+ if (rc < 0)
goto cleanup;
- }
if (rc == 1 && info.cur == info.end &&
info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY)
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
@@ -15412,7 +15433,8 @@ qemuDomainBlockPivot(virConnectPtr conn,
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror->path, format);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (ret < 0) {
/* On failure, qemu abandons the mirror, and reverts back to
@@ -15615,8 +15637,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
speed, mode, async);
- qemuDomainObjExitMonitor(driver, vm);
- if (ret < 0) {
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 ||
+ ret < 0) {
if (mode == BLOCK_JOB_ABORT && disk->mirror)
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
goto endjob;
@@ -15660,7 +15682,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorBlockJobInfo(priv->mon, device, &dummy, NULL);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ break;
if (ret <= 0)
break;
@@ -15764,7 +15787,8 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorBlockJobInfo(priv->mon, device, info, &bandwidth);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (ret < 0)
goto endjob;
@@ -15982,8 +16006,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format,
bandwidth, granularity, buf_size, flags);
virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);
- qemuDomainObjExitMonitor(driver, vm);
- if (ret < 0) {
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) {
qemuDomainPrepareDiskChainElement(driver, vm, mirror,
VIR_DISK_CHAIN_NO_ACCESS);
goto endjob;
@@ -16379,7 +16402,8 @@ qemuDomainBlockCommit(virDomainPtr dom,
ret = qemuMonitorBlockCommit(priv->mon, device,
topPath, basePath, backingPath,
speed);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (mirror) {
if (ret == 0) {
@@ -16473,7 +16497,10 @@ qemuDomainOpenGraphics(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorOpenGraphics(priv->mon, protocol, fd, "graphicsfd",
(flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH) != 0);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
qemuDomainObjEndJob(driver, vm);
cleanup:
@@ -16542,7 +16569,10 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorOpenGraphics(priv->mon, protocol, pair[1],
"graphicsfd",
(flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH));
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
qemuDomainObjEndJob(driver, vm);
if (ret < 0)
goto cleanup;
@@ -16985,7 +17015,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
goto endjob;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply,
supportMaxOptions);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (ret < 0)
goto endjob;
}
@@ -17157,7 +17188,8 @@ qemuDomainGetDiskErrors(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
table = qemuMonitorGetBlockInfo(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (!table)
goto endjob;
@@ -17436,7 +17468,8 @@ qemuDomainPMWakeup(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemWakeup(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -17867,7 +17900,8 @@ qemuDomainSetTime(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
rv = qemuMonitorRTCResetReinjection(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (rv < 0)
goto endjob;
@@ -18512,7 +18546,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
visitBacking);
ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
visitBacking));
- qemuDomainObjExitMonitor(driver, dom);
+ if (qemuDomainObjExitMonitor(driver, dom) < 0)
+ goto cleanup;
if (rc < 0) {
virResetLastError();
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 31494c8..87b843b 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -550,7 +550,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
qemuDomainObjPrivatePtr priv = vm->privateData;
virHashTablePtr stats = NULL;
size_t i;
- int ret = -1;
+ int ret = -1, rc;
/* It is not a bug if there already is a NBD data */
if (!mig->nbd &&
@@ -571,18 +571,11 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
goto cleanup;
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
- false) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
- }
- qemuDomainObjExitMonitor(driver, vm);
-
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domain exited meanwhile"));
+ if (rc < 0)
goto cleanup;
- }
}
if (!disk->info.alias ||
@@ -1655,14 +1648,11 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
if (!port &&
((virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) ||
(qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0))) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
+ goto exit_monitor;
}
- if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
- }
+ if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0)
+ goto exit_monitor;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
}
@@ -1675,6 +1665,10 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
if (ret < 0)
virPortAllocatorRelease(driver->migrationPorts, port);
return ret;
+
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ goto cleanup;
}
/**
@@ -1764,7 +1758,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
goto error;
mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest,
NULL, speed, 0, 0, mirror_flags);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto error;
if (mon_ret < 0)
goto error;
@@ -1785,7 +1780,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
/* explicitly do this *after* we entered the monitor,
* as this is a critical section so we are guaranteed
* priv->job.asyncAbort will not change */
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
@@ -1794,7 +1789,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
}
mon_ret = qemuMonitorBlockJobInfo(priv->mon, diskAlias, &info,
NULL);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
if (mon_ret < 0)
goto error;
@@ -1849,7 +1844,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
BLOCK_JOB_ABORT, true) < 0) {
VIR_WARN("Unable to cancel block-job on '%s'",
diskAlias);
}
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
} else {
VIR_WARN("Unable to enter monitor. No block job cancelled");
}
@@ -2231,6 +2226,7 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
bool wait_for_spice = false;
bool spice_migrated = false;
size_t i = 0;
+ int rc;
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) {
for (i = 0; i < vm->def->ngraphics; i++) {
@@ -2252,12 +2248,11 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
return -1;
- if (qemuMonitorGetSpiceMigrationStatus(priv->mon,
- &spice_migrated) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ rc = qemuMonitorGetSpiceMigrationStatus(priv->mon, &spice_migrated);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
+ if (rc < 0)
return -1;
- }
- qemuDomainObjExitMonitor(driver, vm);
virObjectUnlock(vm);
nanosleep(&ts, NULL);
virObjectLock(vm);
@@ -2287,7 +2282,8 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
}
ret = qemuMonitorGetMigrationStatus(priv->mon, &status);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
if (ret < 0 ||
qemuDomainJobInfoUpdateTime(priv->job.current) < 0)
@@ -3884,7 +3880,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
/* explicitly do this *after* we entered the monitor,
* as this is a critical section so we are guaranteed
* priv->job.asyncAbort will not change */
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
@@ -3892,24 +3888,20 @@ qemuMigrationRun(virQEMUDriverPtr driver,
goto cleanup;
}
- if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
- }
+ if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0)
+ goto exit_monitor;
/* connect to the destination qemu if needed */
if (spec->destType == MIGRATION_DEST_CONNECT_HOST &&
qemuMigrationConnect(driver, vm, spec) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
+ goto exit_monitor;
}
switch (spec->destType) {
case MIGRATION_DEST_HOST:
if (STREQ(spec->dest.host.protocol, "rdma") &&
virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit <<
10) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
+ goto exit_monitor;
}
ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags,
spec->dest.host.protocol,
@@ -3943,17 +3935,12 @@ qemuMigrationRun(virQEMUDriverPtr driver,
VIR_FORCE_CLOSE(spec->dest.fd.qemu);
break;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if (ret < 0)
goto cleanup;
ret = -1;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
- goto cleanup;
- }
-
/* From this point onwards we *must* call cancel to abort the
* migration on source if anything goes wrong */
@@ -4045,6 +4032,10 @@ qemuMigrationRun(virQEMUDriverPtr driver,
return ret;
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ goto cleanup;
+
cancel:
orig_err = virSaveLastError();
@@ -4052,7 +4043,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
qemuMonitorMigrateCancel(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
}
goto cleanup;
@@ -5330,7 +5321,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
qemuMonitorSetMigrationSpeed(priv->mon,
QEMU_DOMAIN_MIG_BANDWIDTH_MAX);
priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
}
if (!virDomainObjIsActive(vm)) {
@@ -5406,11 +5398,11 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
if (virSetCloseExec(pipeFD[1]) < 0) {
virReportSystemError(errno, "%s",
_("Unable to set cloexec flag"));
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto cleanup;
}
if (virCommandRunAsync(cmd, NULL) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto cleanup;
}
rc = qemuMonitorMigrateToFd(priv->mon,
@@ -5425,14 +5417,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
args, path, offset);
}
}
- qemuDomainObjExitMonitor(driver, vm);
-
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
- }
-
if (rc < 0)
goto cleanup;
@@ -5445,7 +5431,7 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
if (virDomainObjIsActive(vm) &&
qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
qemuMonitorMigrateCancel(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
}
goto cleanup;
@@ -5465,7 +5451,7 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
qemuMonitorSetMigrationSpeed(priv->mon, saveMigBandwidth);
priv->migMaxBandwidth = saveMigBandwidth;
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
VIR_FORCE_CLOSE(pipeFD[0]);
--
2.0.4
8:13 a.m.
New subject: [libvirt] [PATCHv2 12/14] Exit early after domain crash in monitor in qemu_driver
<no commit message>?
On 01/07/2015 10:42 AM, Ján Tomko wrote:
---
src/qemu/qemu_driver.c | 131 +++++++++++++++++++++++++++++-----------------
src/qemu/qemu_migration.c | 98 +++++++++++++++-------------------
2 files changed, 125 insertions(+), 104 deletions(-)
Should qemu_migration move to 10/14?
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1781ea9..55d6fb3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1978,7 +1978,8 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int
flags)
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemPowerdown(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
}
endjob:
@@ -2073,7 +2074,8 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
} else {
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemPowerdown(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
if (ret == 0)
qemuDomainSetFakeReboot(driver, vm, isReboot);
@@ -2116,7 +2118,8 @@ qemuDomainReset(virDomainPtr dom, unsigned int flags)
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemReset(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
priv->fakeReboot = false;
@@ -2333,7 +2336,8 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long
newmem,
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetBalloon(priv->mon, newmem);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
Here we have another Audit issue...
virDomainAuditMemory(vm, vm->def->mem.cur_balloon,
newmem, "update",
r == 1);
if (r < 0)
@@ -2415,7 +2419,8 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (r < 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("unable to set balloon driver collection
period"));
@@ -2479,7 +2484,8 @@ static int qemuDomainInjectNMI(virDomainPtr domain, unsigned int
flags)
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorInjectNMI(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
if ret == 0, then @ cleanup we return success
endjob:
qemuDomainObjEndJob(driver, vm);
Hmmm... do we need to call DomainObjEndJob if ExitMonitor fails?
I don't even remember if this was an issue for previous such exits...
@@ -2541,7 +2547,8 @@ static int qemuDomainSendKey(virDomainPtr
domain,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
again, if ret == 0, then we return wrong status possibly... Also I see
another EndJob below...
endjob:
qemuDomainObjEndJob(driver, vm);
> @@ -2596,7 +2603,10 @@ static int
qemuDomainGetInfo(virDomainPtr dom,
> } else {
> qemuDomainObjEnterMonitor(driver, vm);
> err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> + qemuDomainObjEndJob(driver, vm);
Well, I think I have my answer. You may want to go back through all the
changes to see if any are missed...
+ goto cleanup;
+ }
}
qemuDomainObjEndJob(driver, vm);
@@ -3492,7 +3502,7 @@ static int qemuDumpToFd(virQEMUDriverPtr driver, virDomainObjPtr
vm,
ret = qemuMonitorDumpToFd(priv->mon, fd, dumpformat);
cleanup:
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
return ret;
}
@@ -3691,7 +3701,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom,
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSystemReset(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
if (resume && virDomainObjIsActive(vm)) {
@@ -3788,10 +3798,11 @@ qemuDomainScreenshot(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorScreendump(priv->mon, tmp) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto endjob;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (VIR_CLOSE(tmp_fd) < 0) {
virReportSystemError(errno, _("unable to close %s"), tmp);
@@ -4216,7 +4227,8 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &guestFilter);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
Even though it's a void function, I think instead of goto endjob, it
should be ret = -1...
> if (ret < 0)
> goto endjob;
>
> @@ -6227,7 +6239,8 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + err = -1;
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -10083,10 +10096,11 @@ qemuDomainBlockResize(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> if (qemuMonitorBlockResize(priv->mon, device, size) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto endjob;
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
>
> ret = 0;
>
> @@ -10740,7 +10754,8 @@ qemuDomainMemoryStats(virDomainPtr dom,
> qemuDomainObjPrivatePtr priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> if (ret >= 0 && ret < nr_stats) {
> long rss;
> @@ -10874,16 +10889,17 @@ qemuDomainMemoryPeek(virDomainPtr dom,
> qemuDomainObjEnterMonitor(driver, vm);
> if (flags == VIR_MEMORY_VIRTUAL) {
> if (qemuMonitorSaveVirtualMemory(priv->mon, offset, size, tmp) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto endjob;
> }
> } else {
> if (qemuMonitorSavePhysicalMemory(priv->mon, offset, size, tmp) < 0)
{
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto endjob;
> }
> }
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
>
> /* Read the memory file into buffer. */
> if (saferead(fd, buffer, size) == (ssize_t) -1) {
> @@ -11121,7 +11137,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
> ret = qemuMonitorGetBlockExtent(priv->mon,
> disk->info.alias,
> &src->allocation);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
> }
>
> if (ret == 0) {
> @@ -12361,7 +12378,8 @@ static int qemuDomainAbortJob(virDomainPtr dom)
> qemuDomainObjAbortAsyncJob(vm);
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorMigrateCancel(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
> @@ -12404,7 +12422,8 @@
qemuDomainMigrateSetMaxDowntime(virDomainPtr dom,
> VIR_DEBUG("Setting migration downtime to %llums", downtime);
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSetMigrationDowntime(priv->mon, downtime);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
> @@ -12457,7 +12476,8 @@
qemuDomainMigrateGetCompressionCache(virDomainPtr dom,
> ret = qemuMonitorGetMigrationCacheSize(priv->mon, cacheSize);
> }
>
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
> @@ -12511,7 +12531,8 @@
qemuDomainMigrateSetCompressionCache(virDomainPtr dom,
> ret = qemuMonitorSetMigrationCacheSize(priv->mon, cacheSize);
> }
>
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
> @@ -12561,7 +12582,8 @@
qemuDomainMigrateSetMaxSpeed(virDomainPtr dom,
> VIR_DEBUG("Setting migration bandwidth to %luMbs", bandwidth);
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSetMigrationSpeed(priv->mon, bandwidth);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
>
> if (ret == 0)
> priv->migMaxBandwidth = bandwidth;
> @@ -15006,7 +15028,10 @@ static int qemuDomainQemuMonitorCommand(virDomainPtr domain,
const char *cmd,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorArbitraryCommand(priv->mon, cmd, result, hmp);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> + ret = -1;
> + goto endjob;
This goto seems superfluous... especially compared to other recent
changes. Although seemingly correct, it's handled differently and it's
so much easier when things are more consistent...
Of course each of them could just have ret = qemuDomainobjExitMonitor()
- or at least these inline cases where endjob: is almost always the next
line.
> + }
endjob:
qemuDomainObjEndJob(driver, vm);
> @@ -15328,14 +15353,10 @@
qemuDomainBlockPivot(virConnectPtr conn,
> if (!disk->mirrorState) {
> qemuDomainObjEnterMonitor(driver, vm);
> rc = qemuMonitorBlockJobInfo(priv->mon, device, &info, NULL);
> - qemuDomainObjExitMonitor(driver, vm);
> - if (rc < 0)
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("domain is not running"));
> + if (rc < 0)
> goto cleanup;
> - }
> if (rc == 1 && info.cur == info.end &&
> info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY)
> disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
> @@ -15412,7 +15433,8 @@ qemuDomainBlockPivot(virConnectPtr conn,
> disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror->path,
format);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
In this case, if ret == 0, then we return success incorrectly. Perhaps
we should set ret = qemuDomainObjExitMonitor here since the next if
handles < 0, although after that there's a SaveStatus, which I wonder if
we really want to do in the case where ret < 0.
if (ret < 0) {
/* On failure, qemu abandons the mirror, and reverts back to
@@ -15615,8 +15637,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
speed, mode, async);
- qemuDomainObjExitMonitor(driver, vm);
- if (ret < 0) {
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 ||
+ ret < 0) {
Again - if ret == 0, then what?
FWIW: this is a construct I was going to suggest for many of the other
failure checks where both would fall into an error path, until I started
thinking about the ret == 0 and then Exit == failure case...
if (mode == BLOCK_JOB_ABORT && disk->mirror)
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
goto endjob;
@@ -15660,7 +15682,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorBlockJobInfo(priv->mon, device, &dummy, NULL);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ break;
Here again, ret == 0 and we're breaking - although the next check makes
me wonder... Furthermore about 10 lines or so dow there's a
virDomainObjIsActive check - is that now not necessary like other places?
if (ret <= 0)
break;
@@ -15764,7 +15787,8 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorBlockJobInfo(priv->mon, device, info, &bandwidth);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
Another ret == 0 possibility
if (ret < 0)
goto endjob;
@@ -15982,8 +16006,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format,
bandwidth, granularity, buf_size, flags);
virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);
OK - here we Audit right after the DriveMirror, then exit the Monitor...
- qemuDomainObjExitMonitor(driver, vm);
- if (ret < 0) {
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) {
Another place where ret == 0 could be true...
qemuDomainPrepareDiskChainElement(driver, vm, mirror,
VIR_DISK_CHAIN_NO_ACCESS);
goto endjob;
@@ -16379,7 +16402,8 @@ qemuDomainBlockCommit(virDomainPtr dom,
ret = qemuMonitorBlockCommit(priv->mon, device,
topPath, basePath, backingPath,
speed);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
Againt ret == 0 possible.
if (mirror) {
if (ret == 0) {
@@ -16473,7 +16497,10 @@ qemuDomainOpenGraphics(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorOpenGraphics(priv->mon, protocol, fd, "graphicsfd",
(flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH) !=
0);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
We're not going call ObjEndJob?
+ }
qemuDomainObjEndJob(driver, vm);
cleanup:
@@ -16542,7 +16569,10 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorOpenGraphics(priv->mon, protocol, pair[1],
"graphicsfd",
(flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH));
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
Again no ObjEndJob?
+ }
qemuDomainObjEndJob(driver, vm);
if (ret < 0)
goto cleanup;
@@ -16985,7 +17015,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
goto endjob;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply,
supportMaxOptions);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
We could have ret == 0 and goto endjob.
> if (ret < 0)
> goto endjob;
> }
> @@ -17157,7 +17188,8 @@ qemuDomainGetDiskErrors(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> table = qemuMonitorGetBlockInfo(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
> if (!table)
> goto endjob;
>
> @@ -17436,7 +17468,8 @@ qemuDomainPMWakeup(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSystemWakeup(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
endjob:
qemuDomainObjEndJob(driver, vm);
> @@ -17867,7 +17900,8 @@
qemuDomainSetTime(virDomainPtr dom,
>
> qemuDomainObjEnterMonitor(driver, vm);
> rv = qemuMonitorRTCResetReinjection(priv->mon);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
>
> if (rv < 0)
> goto endjob;
> @@ -18512,7 +18546,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> visitBacking);
> ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
> visitBacking));
> - qemuDomainObjExitMonitor(driver, dom);
> + if (qemuDomainObjExitMonitor(driver, dom) < 0)
> + goto cleanup;
>
> if (rc < 0) {
> virResetLastError();
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 31494c8..87b843b 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -550,7 +550,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
> qemuDomainObjPrivatePtr priv = vm->privateData;
> virHashTablePtr stats = NULL;
> size_t i;
> - int ret = -1;
> + int ret = -1, rc;
>
> /* It is not a bug if there already is a NBD data */
> if (!mig->nbd &&
> @@ -571,18 +571,11 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
> goto cleanup;
>
> qemuDomainObjEnterMonitor(driver, vm);
> - if (qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
> - false) < 0) {
> - qemuDomainObjExitMonitor(driver, vm);
> + rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> - }
> - qemuDomainObjExitMonitor(driver, vm);
> -
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("domain exited meanwhile"));
> + if (rc < 0)
This is the kind of construct where I thought a :
if (qemuDomainObjExitMonitor(driver,vm) < 0 || rc < 0)
would fit... It doesn't matter to me - although consistency is nice too.
It's almost like a macro could be generated that would check the status
of qemuMonitor* call and the status of *ExitMonitor using another 'goto'
type parameter in order to consistently handle each of these...
goto cleanup;
- }
}
if (!disk->info.alias ||
@@ -1655,14 +1648,11 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
if (!port &&
((virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) ||
(qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0))) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
+ goto exit_monitor;
}
- if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
- }
+ if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0)
+ goto exit_monitor;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
}
@@ -1675,6 +1665,10 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
if (ret < 0)
virPortAllocatorRelease(driver->migrationPorts, port);
return ret;
+
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ goto cleanup;
}
/**
@@ -1764,7 +1758,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
goto error;
mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest,
NULL, speed, 0, 0, mirror_flags);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto error;
if (mon_ret < 0)
goto error;
@@ -1785,7 +1780,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
/* explicitly do this *after* we entered the monitor,
* as this is a critical section so we are guaranteed
* priv->job.asyncAbort will not change */
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
@@ -1794,7 +1789,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
}
mon_ret = qemuMonitorBlockJobInfo(priv->mon, diskAlias, &info,
NULL);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
if (mon_ret < 0)
goto error;
@@ -1849,7 +1844,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
BLOCK_JOB_ABORT, true) < 0) {
VIR_WARN("Unable to cancel block-job on '%s'",
diskAlias);
}
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
The context of this is within a while (lastGood) loop in an error path -
theoretically the failure is dead guest, so what good is contining the
loop? I do agree other paths are mainly ignoring errors too, but it
doesn't seem necessary to continue if the guest died anyway.
} else {
VIR_WARN("Unable to enter monitor. No block job cancelled");
}
@@ -2231,6 +2226,7 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
bool wait_for_spice = false;
bool spice_migrated = false;
size_t i = 0;
+ int rc;
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) {
for (i = 0; i < vm->def->ngraphics; i++) {
@@ -2252,12 +2248,11 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
return -1;
- if (qemuMonitorGetSpiceMigrationStatus(priv->mon,
- &spice_migrated) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ rc = qemuMonitorGetSpiceMigrationStatus(priv->mon, &spice_migrated);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
+ if (rc < 0)
return -1;
- }
- qemuDomainObjExitMonitor(driver, vm);
virObjectUnlock(vm);
nanosleep(&ts, NULL);
virObjectLock(vm);
@@ -2287,7 +2282,8 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
}
ret = qemuMonitorGetMigrationStatus(priv->mon, &status);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
if (ret < 0 ||
qemuDomainJobInfoUpdateTime(priv->job.current) < 0)
@@ -3884,7 +3880,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
/* explicitly do this *after* we entered the monitor,
* as this is a critical section so we are guaranteed
* priv->job.asyncAbort will not change */
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
@@ -3892,24 +3888,20 @@ qemuMigrationRun(virQEMUDriverPtr driver,
goto cleanup;
}
- if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
- }
+ if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0)
+ goto exit_monitor;
/* connect to the destination qemu if needed */
if (spec->destType == MIGRATION_DEST_CONNECT_HOST &&
qemuMigrationConnect(driver, vm, spec) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
+ goto exit_monitor;
}
switch (spec->destType) {
case MIGRATION_DEST_HOST:
if (STREQ(spec->dest.host.protocol, "rdma") &&
virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit <<
10) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
- goto cleanup;
+ goto exit_monitor;
}
ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags,
spec->dest.host.protocol,
@@ -3943,17 +3935,12 @@ qemuMigrationRun(virQEMUDriverPtr driver,
VIR_FORCE_CLOSE(spec->dest.fd.qemu);
break;
}
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
Here is another case where if ret == 0, then goto cleanup doesn't seem
right.
if (ret < 0)
goto cleanup;
ret = -1;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
- goto cleanup;
- }
-
/* From this point onwards we *must* call cancel to abort the
* migration on source if anything goes wrong */
@@ -4045,6 +4032,10 @@ qemuMigrationRun(virQEMUDriverPtr driver,
return ret;
+ exit_monitor:
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
should a "ret = -1;" be added here?
I think the rest seem to be OK - my eyes got weary and I hope I didn't
miss any cases. It may be worth going back through the patches 9-13
when you've made all the changes in order to ensure things are more
consistent and that nothing was missed.
ACK with changes -
John
+ goto cleanup;
+
cancel:
orig_err = virSaveLastError();
@@ -4052,7 +4043,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
qemuMonitorMigrateCancel(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
}
goto cleanup;
@@ -5330,7 +5321,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
qemuMonitorSetMigrationSpeed(priv->mon,
QEMU_DOMAIN_MIG_BANDWIDTH_MAX);
priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
}
if (!virDomainObjIsActive(vm)) {
@@ -5406,11 +5398,11 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
if (virSetCloseExec(pipeFD[1]) < 0) {
virReportSystemError(errno, "%s",
_("Unable to set cloexec flag"));
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto cleanup;
}
if (virCommandRunAsync(cmd, NULL) < 0) {
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto cleanup;
}
rc = qemuMonitorMigrateToFd(priv->mon,
@@ -5425,14 +5417,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
args, path, offset);
}
}
- qemuDomainObjExitMonitor(driver, vm);
-
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
- }
-
if (rc < 0)
goto cleanup;
@@ -5445,7 +5431,7 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
if (virDomainObjIsActive(vm) &&
qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
qemuMonitorMigrateCancel(priv->mon);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
}
goto cleanup;
@@ -5465,7 +5451,7 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
qemuMonitorSetMigrationSpeed(priv->mon, saveMigBandwidth);
priv->migMaxBandwidth = saveMigBandwidth;
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
VIR_FORCE_CLOSE(pipeFD[0]);
9:42 a.m.
New subject: [libvirt] [PATCHv2 13/14] Exit early after domain crash in monitor on snapshots
---
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_driver.c | 18 ++++++------------
2 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 82d0c91..3d56eb8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2313,7 +2313,7 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
/* we continue on even in the face of error */
qemuMonitorDeleteSnapshot(priv->mon, snap->def->name);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 55d6fb3..a3d8983 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12896,7 +12896,8 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
}
ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
if (ret < 0)
goto cleanup;
@@ -13417,12 +13418,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr
driver,
ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source,
formatStr, reuse);
if (!actions) {
- qemuDomainObjExitMonitor(driver, vm);
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domain crashed while taking the snapshot"));
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
ret = -1;
- }
}
virDomainAuditDisk(vm, disk->src, snap->src, "snapshot", ret >=
0);
@@ -13560,12 +13557,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
if (ret == 0) {
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
ret = qemuMonitorTransaction(priv->mon, actions);
- qemuDomainObjExitMonitor(driver, vm);
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domain crashed while taking the
snapshot"));
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
ret = -1;
- }
} else {
/* failed to enter monitor, clean stuff up and quit */
ret = -1;
@@ -14656,7 +14649,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
qemuDomainObjEnterMonitor(driver, vm);
rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (rc < 0) {
/* XXX resume domain if it was running before the
* failed loadvm attempt? */
--
2.0.4
8:43 a.m.
New subject: [libvirt] [PATCHv2 13/14] Exit early after domain crash in monitor on snapshots
<no commit message>
On 01/07/2015 10:42 AM, Ján Tomko wrote:
---
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_driver.c | 18 ++++++------------
2 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 82d0c91..3d56eb8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2313,7 +2313,7 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
/* we continue on even in the face of error */
qemuMonitorDeleteSnapshot(priv->mon, snap->def->name);
- qemuDomainObjExitMonitor(driver, vm);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 55d6fb3..a3d8983 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12896,7 +12896,8 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
}
ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
if (ret < 0)
goto cleanup;
@@ -13417,12 +13418,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr
driver,
ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source,
formatStr, reuse);
if (!actions) {
- qemuDomainObjExitMonitor(driver, vm);
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domain crashed while taking the snapshot"));
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
ret = -1;
- }
}
In this case we do fall through to Audit - what the "strange" part of
the Audit is though is we've changed the value of 'ret' if the
ExitMonitor fails. Could a 'successful' DiskSnapshot ret value be
changed into a failure because the guest died for some reason. Could
anything else jump in (I've totally lost track now :-))
If that's expected, fine, but I thought it worthy to point out. Although
I do "suspect" it could be reasonable to expect that the crash was
because of taking the snapshot, but who knows for sure.
ACK in general...
Now that I'm done with the various callers. It's been hard to keep track
in my own mind of the consistency between each routine and what I may
have caught along the way. The following 4 do spring to mind though
1. Audit
I think the Audits should occur. Whether they "all" happen right
after the qemuMonitor* call while we still have the lock or whether
they're done afterwards doesn't matter to me.
2. EndJob
I started noticing some cases where EndJob was called and other times
where it was missed. Of course I didn't go back to earlier patches to
check all cases, but that might be something you want to do to ensure
we're not leaving somewhere without the EndJob
3. Possibility of ret = qemuMonitor* == 0 and ExitMonitor == -1
While I agree it's highly likely that ret == -1 prior to ExitMonitor
calls where sometimes ret = -1 is done and other times it's not. Still,
if it is possible, then it needs to be accounted for. It doesn't seem
so, but to have some call sequences do things one way while others do it
differently makes it difficult to keep track.
4. Consistency
I realize the inconsistencies existed before these patches, but since
you're tackling all the callers in this series of patches - it may be
nice to make things consistent. That way the next person who copies what
some command does will then hopefully create consistent code. Another
option is to make a macro that handles the ExitMonitor and goto's...
John
virDomainAuditDisk(vm, disk->src, snap->src, "snapshot", ret >=
0);
@@ -13560,12 +13557,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
if (ret == 0) {
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
ret = qemuMonitorTransaction(priv->mon, actions);
- qemuDomainObjExitMonitor(driver, vm);
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domain crashed while taking the
snapshot"));
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
ret = -1;
- }
} else {
/* failed to enter monitor, clean stuff up and quit */
ret = -1;
@@ -14656,7 +14649,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
qemuDomainObjEnterMonitor(driver, vm);
rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name);
- qemuDomainObjExitMonitor(driver, vm);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto endjob;
if (rc < 0) {
/* XXX resume domain if it was running before the
* failed loadvm attempt? */
8:27 a.m.
New subject: [libvirt] [PATCHv2 13/14] Exit early after domain crash in monitor on snapshots
On 01/13/2015 03:43 PM, John Ferlan wrote:
<no commit message>
On 01/07/2015 10:42 AM, Ján Tomko wrote:
> ---
> src/qemu/qemu_domain.c | 2 +-
> src/qemu/qemu_driver.c | 18 ++++++------------
> 2 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 82d0c91..3d56eb8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2313,7 +2313,7 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
> qemuDomainObjEnterMonitor(driver, vm);
> /* we continue on even in the face of error */
> qemuMonitorDeleteSnapshot(priv->mon, snap->def->name);
> - qemuDomainObjExitMonitor(driver, vm);
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> }
> }
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 55d6fb3..a3d8983 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -12896,7 +12896,8 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
> }
>
> ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
> if (ret < 0)
> goto cleanup;
>
> @@ -13417,12 +13418,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr
driver,
> ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source,
> formatStr, reuse);
> if (!actions) {
> - qemuDomainObjExitMonitor(driver, vm);
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("domain crashed while taking the
snapshot"));
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> ret = -1;
> - }
> }
In this case we do fall through to Audit - what the "strange" part of
the Audit is though is we've changed the value of 'ret' if the
ExitMonitor fails. Could a 'successful' DiskSnapshot ret value be
changed into a failure because the guest died for some reason. Could
anything else jump in (I've totally lost track now :-))
If that's expected, fine, but I thought it worthy to point out. Although
I do "suspect" it could be reasonable to expect that the crash was
because of taking the snapshot, but who knows for sure.
ACK in general...
Now that I'm done with the various callers. It's been hard to keep track
in my own mind of the consistency between each routine and what I may
have caught along the way. The following 4 do spring to mind though
1. Audit
I think the Audits should occur. Whether they "all" happen right
after the qemuMonitor* call while we still have the lock or whether
they're done afterwards doesn't matter to me.
As of now, the audits are broken because they possibly access freed data.
While the vm->def that will be replaced on domain crash still contains
the basic data needed for audit, the device pointers into vm->def will be stale.
The aim of this series is to remove the crash possibility by not using those
pointers.
While we are in monitor, the domain object is unlocked, so even accessing
vm->def should not be safe. After exiting the monitor, we'd need a copy of the
device that was not a part of vm->def, which we have for attach and
things like setvcpus, where the auditted 'device' is just an int, but not for
detach.
Regardless of this series, the audit logging is inconsistent and should be
IMHO addressed separately.
2. EndJob
I started noticing some cases where EndJob was called and other times
where it was missed. Of course I didn't go back to earlier patches to
check all cases, but that might be something you want to do to ensure
we're not leaving somewhere without the EndJob
3. Possibility of ret = qemuMonitor* == 0 and ExitMonitor == -1
While I agree it's highly likely that ret == -1 prior to ExitMonitor
calls where sometimes ret = -1 is done and other times it's not. Still,
if it is possible, then it needs to be accounted for. It doesn't seem
so, but to have some call sequences do things one way while others do it
differently makes it difficult to keep track.
Ouch, that's probably a result of me spending less time per line on the last 6
patches. I'll take another look before pushing/resending. (So far I've got
patches 1, 2, 6-8 queued as I believe there were no audit issues there)
4. Consistency
I realize the inconsistencies existed before these patches, but since
you're tackling all the callers in this series of patches - it may be
nice to make things consistent. That way the next person who copies what
some command does will then hopefully create consistent code. Another
option is to make a macro that handles the ExitMonitor and goto's...
I'm not sure I understand what should be consistent and can't imagine a
universal and readable macro for that.
For the audit, if qemuMonitor* succeeds but ExitMonitor fails, it would be
nice to log 'true', but that would be nicer in a separate series adressing all
the audit calls.
As for the ignore_value vs. goto cleanup, I try to use goto when possible,
because ingore value is harder to read to me.
Jan
John
>
> virDomainAuditDisk(vm, disk->src, snap->src, "snapshot", ret
>= 0);
> @@ -13560,12 +13557,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
> if (ret == 0) {
> if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
> ret = qemuMonitorTransaction(priv->mon, actions);
> - qemuDomainObjExitMonitor(driver, vm);
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("domain crashed while taking the
snapshot"));
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> ret = -1;
> - }
> } else {
> /* failed to enter monitor, clean stuff up and quit */
> ret = -1;
> @@ -14656,7 +14649,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> }
> qemuDomainObjEnterMonitor(driver, vm);
> rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name);
> - qemuDomainObjExitMonitor(driver, vm);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto endjob;
> if (rc < 0) {
> /* XXX resume domain if it was running before the
> * failed loadvm attempt? */
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:42 a.m.
New subject: [libvirt] [PATCHv2 14/14] Add ATTRIBUTE_RETURN_CHECK to qemuDomainObjExitMonitor
---
src/qemu/qemu_domain.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index fd91d83..b2c3881 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -247,7 +247,8 @@ void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
virDomainObjPtr obj)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
+ ATTRIBUTE_RETURN_CHECK;
int qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainAsyncJob asyncJob)
--
2.0.4
7:19 p.m.
On 01/07/2015 10:42 AM, Ján Tomko wrote:
Patches 3-8 fix possible crash/invalid memory access if QEMU crashes
while we're in the monitor.
Patches 9-13 change all other callers of qemuDomainObjExitMonitor
to exit early in that case, but should not fix any real issues.
They are necessary to turn on ATTRIBUTE_RETURN_CHECK for the ExitMonitor
call.
My brain has saturated in the middle of patch 12.... I'll pick this up
in the morning
John
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
Ján Tomko (14):
Check for domain liveness in qemuDomainObjExitMonitor
Mark the domain as active in qemuhotplugtest
Fix vmdef usage after domain crash in monitor on device removal
Fix vmdef usage after domain crash in monitor on device detach
Fix vmdef usage after domain crash in monitor on device attach
Fix vmdef usage while in monitor in qemuDomainHotplugVcpus
Fix vmdef usage while in monitor in BlockStat* APIs
Fix vmdef usage while in monitor in qemu process
Exit early after domain crash in monitor on device hotplug
Exit early after domain crash in monitor on migration
Exit early after domain crash in monitor in qemu_process
Exit early after domain crash in monitor in qemu_driver
Exit early after domain crash in monitor on snapshots
Add ATTRIBUTE_RETURN_CHECK to qemuDomainObjExitMonitor
src/qemu/THREADS.txt | 5 ++
src/qemu/qemu_domain.c | 27 +++++--
src/qemu/qemu_domain.h | 7 +-
src/qemu/qemu_driver.c | 196 +++++++++++++++++++++++++++++-----------------
src/qemu/qemu_hotplug.c | 183 ++++++++++++++++++++++++++-----------------
src/qemu/qemu_hotplug.h | 6 +-
src/qemu/qemu_migration.c | 137 ++++++++++++++++----------------
src/qemu/qemu_process.c | 128 +++++++++++++++++-------------
tests/qemuhotplugtest.c | 6 ++
9 files changed, 410 insertions(+), 285 deletions(-)
3600
days inactive
3607
days old
35 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko