[libvirt] [PATCH] qemu: respect locking rules
Eric Blake
eblake at redhat.com
Tue Mar 15 02:38:09 UTC 2011
THREADS.txt states that the contents of vm should not be read or
modified while the vm lock is not held, but that the lock must not
be held while performing a monitor command. This fixes all the
offenders that I could find.
* src/qemu/qemu_process.c (qemuProcessStartCPUs)
(qemuProcessInitPasswords, qemuProcessStart): Don't modify or
refer to vm state outside lock.
* src/qemu/qemu_driver.c (qemudDomainHotplugVcpus): Likewise.
* src/qemu/qemu_hotplug.c (qemuDomainChangeGraphicsPasswords):
Likewise.
---
Most of these just move the vm manipulation outside the monitor lock.
qemu_hotplug is a case of checking if the vm is active in between two
monitor commands while the monitor lock was not dropped. Technically,
this was wasted - the qemu process might have stopped (independently,
such as from a guest shutdown), but as long as we hold the monitor
lock, the monitor object will react correctly even if the underlying
qemu process goes away in between the two monitor commands. The
alternative would have been to drop monitor lock, regain vm lock, keep
the check that vm is still active but now under the correct lock, then
regain monitor lock and drop vm lock.
My only question is whether qemuProcessStartCPUs needs to be careful
to check if the VM is still active when it regains the vm lock, since
it is possible that the guest shutdown in the window between the
monitor command resuming the CPUs and when the vm lock is regained -
that is, blindly setting vm->state to VIR_DOMAIN_RUNNING seems fishy
if the vm went away during the window. However, when I tested this
under gdb, by setting a breakpoint in that window and causing the
guest to shutdown, it looked like everything recovered gracefully.
And if we do start checking after a monitor exit that the vm is still
active before changing state of the vm object, then we should probably
adopt that convention everywhere.
src/qemu/qemu_driver.c | 12 +++++++-----
src/qemu/qemu_hotplug.c | 7 -------
src/qemu/qemu_process.c | 18 +++++++++++-------
3 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dac2bf2..c8870b1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2553,14 +2553,15 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus)
int i, rc = 1;
int ret = -1;
int oldvcpus = vm->def->vcpus;
+ int vcpus = oldvcpus;
qemuDomainObjEnterMonitor(vm);
/* We need different branches here, because we want to offline
* in reverse order to onlining, so any partial fail leaves us in a
* reasonably sensible state */
- if (nvcpus > vm->def->vcpus) {
- for (i = vm->def->vcpus ; i < nvcpus ; i++) {
+ if (nvcpus > vcpus) {
+ for (i = vcpus ; i < nvcpus ; i++) {
/* Online new CPU */
rc = qemuMonitorSetCPU(priv->mon, i, 1);
if (rc == 0)
@@ -2568,10 +2569,10 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus)
if (rc < 0)
goto cleanup;
- vm->def->vcpus++;
+ vcpus++;
}
} else {
- for (i = vm->def->vcpus - 1 ; i >= nvcpus ; i--) {
+ for (i = vcpus - 1 ; i >= nvcpus ; i--) {
/* Offline old CPU */
rc = qemuMonitorSetCPU(priv->mon, i, 0);
if (rc == 0)
@@ -2579,7 +2580,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus)
if (rc < 0)
goto cleanup;
- vm->def->vcpus--;
+ vcpus--;
}
}
@@ -2587,6 +2588,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus)
cleanup:
qemuDomainObjExitMonitor(vm);
+ vm->def->vcpus = vcpus;
qemuAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);
return ret;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e4ba526..e1d9d29 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1836,13 +1836,6 @@ qemuDomainChangeGraphicsPasswords(struct qemud_driver *driver,
if (ret != 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- ret = -1;
- qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
- goto cleanup;
- }
-
if (auth->expires) {
time_t lifetime = auth->validTo - now;
if (lifetime <= 0)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 740684a..793a43c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1008,8 +1008,8 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver,
if (paths == NULL)
goto cleanup;
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
qemuDomainObjPrivatePtr priv = vm->privateData;
+ qemuDomainObjEnterMonitorWithDriver(driver, vm);
ret = qemuMonitorGetPtyPaths(priv->mon, paths);
qemuDomainObjExitMonitorWithDriver(driver, vm);
@@ -1175,6 +1175,7 @@ qemuProcessInitPasswords(virConnectPtr conn,
for (i = 0 ; i < vm->def->ndisks ; i++) {
char *secret;
size_t secretLen;
+ const char *alias;
if (!vm->def->disks[i]->encryption ||
!vm->def->disks[i]->src)
@@ -1185,10 +1186,9 @@ qemuProcessInitPasswords(virConnectPtr conn,
&secret, &secretLen) < 0)
goto cleanup;
+ alias = vm->def->disks[i]->info.alias;
qemuDomainObjEnterMonitorWithDriver(driver, vm);
- ret = qemuMonitorSetDrivePassphrase(priv->mon,
- vm->def->disks[i]->info.alias,
- secret);
+ ret = qemuMonitorSetDrivePassphrase(priv->mon, alias, secret);
VIR_FREE(secret);
qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret < 0)
@@ -1727,17 +1727,19 @@ qemuProcessPrepareMonitorChr(struct qemud_driver *driver,
}
-int qemuProcessStartCPUs(struct qemud_driver *driver, virDomainObjPtr vm, virConnectPtr conn)
+int
+qemuProcessStartCPUs(struct qemud_driver *driver, virDomainObjPtr vm,
+ virConnectPtr conn)
{
int ret;
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuDomainObjEnterMonitorWithDriver(driver, vm);
ret = qemuMonitorStartCPUs(priv->mon, conn);
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret == 0) {
vm->state = VIR_DOMAIN_RUNNING;
}
- qemuDomainObjExitMonitorWithDriver(driver, vm);
return ret;
}
@@ -1901,6 +1903,7 @@ int qemuProcessStart(virConnectPtr conn,
qemuDomainObjPrivatePtr priv = vm->privateData;
virCommandPtr cmd = NULL;
struct qemuProcessHookData hookData;
+ unsigned long cur_balloon;
hookData.conn = conn;
hookData.vm = vm;
@@ -2210,8 +2213,9 @@ int qemuProcessStart(virConnectPtr conn,
}
VIR_DEBUG0("Setting initial memory amount");
+ cur_balloon = vm->def->mem.cur_balloon;
qemuDomainObjEnterMonitorWithDriver(driver, vm);
- if (qemuMonitorSetBalloon(priv->mon, vm->def->mem.cur_balloon) < 0) {
+ if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) {
qemuDomainObjExitMonitorWithDriver(driver, vm);
goto cleanup;
}
--
1.7.4
More information about the libvir-list
mailing list