On Mon, Mar 14, 2011 at 08:38:09PM -0600, Eric Blake wrote:
> 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;
> }
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list