On Fri, Apr 23, 2010 at 01:27:45PM -0400, Chris Lalancette wrote:
> In particular I was forgetting to take the qemuMonitorPrivatePtr
> lock (via qemuDomainObjBeginJob), which would cause problems
> if two users tried to access the same domain at the same time.
> This patch also fixes a problem where I was forgetting to remove
> a transient domain from the list of domains.
>
> Thanks to Stephen Shaw for pointing out the problem and testing
> out the initial patch.
>
> Signed-off-by: Chris Lalancette <clalance@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 45 +++++++++++++++++++++++++++++++--------------
> 1 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1ef15dd..2f889d9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10690,7 +10690,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
> virDomainSnapshotPtr snapshot = NULL;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virDomainSnapshotDefPtr def;
> - qemuDomainObjPrivatePtr priv;
> const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL };
> int i;
>
> @@ -10757,14 +10756,19 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
> }
> }
> else {
> + qemuDomainObjPrivatePtr priv;
> + int ret;
> +
> + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> + goto cleanup;
> priv = vm->privateData;
> qemuDomainObjEnterMonitorWithDriver(driver, vm);
> - if (qemuMonitorCreateSnapshot(priv->mon, def->name) < 0) {
> - qemuDomainObjExitMonitorWithDriver(driver, vm);
> - goto cleanup;
> - }
> + ret = qemuMonitorCreateSnapshot(priv->mon, def->name);
> qemuDomainObjExitMonitorWithDriver(driver, vm);
> -
> + if (qemuDomainObjEndJob(vm) == 0)
> + vm = NULL;
> + if (ret < 0)
> + goto cleanup;
> }
>
> snap->def->state = vm->state;
> @@ -11037,18 +11041,18 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name);
> qemuDomainObjExitMonitorWithDriver(driver, vm);
> if (rc < 0)
> - goto cleanup;
> + goto endjob;
> }
> else {
> if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0)
> - goto cleanup;
> + goto endjob;
>
> rc = qemudStartVMDaemon(snapshot->domain->conn, driver, vm, NULL,
> -1);
> if (qemuDomainSnapshotSetInactive(vm, driver->snapshotDir) < 0)
> - goto cleanup;
> + goto endjob;
> if (rc < 0)
> - goto cleanup;
> + goto endjob;
> }
>
> if (snap->def->state == VIR_DOMAIN_PAUSED) {
> @@ -11060,7 +11064,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> rc = qemuMonitorStopCPUs(priv->mon);
> qemuDomainObjExitMonitorWithDriver(driver, vm);
> if (rc < 0)
> - goto cleanup;
> + goto endjob;
> }
>
> event = virDomainEventNewFromObj(vm,
> @@ -11083,20 +11087,26 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
> + if (!vm->persistent) {
> + if (qemuDomainObjEndJob(vm) > 0)
> + virDomainRemoveInactive(&driver->domains, vm);
> + vm = NULL;
> + }
> }
>
> if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0)
> - goto cleanup;
> + goto endjob;
> }
>
> vm->state = snap->def->state;
>
> ret = 0;
>
> -cleanup:
> +endjob:
> if (vm && qemuDomainObjEndJob(vm) == 0)
> vm = NULL;
>
> +cleanup:
> if (event)
> qemuDomainEventQueue(driver, event);
> if (vm)
> @@ -11250,6 +11260,9 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> goto cleanup;
> }
>
> + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> + goto cleanup;
> +
> if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) {
> rem.driver = driver;
> rem.vm = vm;
> @@ -11258,11 +11271,15 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardChildren,
> &rem);
> if (rem.err < 0)
> - goto cleanup;
> + goto endjob;
> }
>
> ret = qemuDomainSnapshotDiscard(driver, vm, snap);
>
> +endjob:
> + if (qemuDomainObjEndJob(vm) == 0)
> + vm = NULL;
> +
> cleanup:
> if (vm)
> virDomainObjUnlock(vm);
ACK, looks fine to me,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list