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@redhat.com> --- 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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 04/23/2010 01:57 PM, Matthias Bolte wrote:

2010/4/23 Chris Lalancette <clalance@redhat.com>:

...

While running libvirtd under valgrind and doing some snapshot testing I noticed that we would always leak a connection reference. The problem was actually that we were leaking a domain reference in the libvirtd remote snapshot code, which was in turn causing a leaked connection reference. Fix the situation by explicitly taking and dropping a domain reference where we need it.

Signed-off-by: Chris Lalancette <clalance@redhat.com> --- daemon/remote.c | 107 +++++++++++++++++++++++++++++++++--------------------- 1 files changed, 65 insertions(+), 42 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 738799c..bb8c28c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -68,7 +68,7 @@ static virStoragePoolPtr get_nonnull_storage_pool (virConnectPtr conn, remote_no static virStorageVolPtr get_nonnull_storage_vol (virConnectPtr conn, remote_nonnull_storage_vol vol); static virSecretPtr get_nonnull_secret (virConnectPtr conn, remote_nonnull_secret secret); static virNWFilterPtr get_nonnull_nwfilter (virConnectPtr conn, remote_nonnull_nwfilter nwfilter); -static virDomainSnapshotPtr get_nonnull_domain_snapshot (virConnectPtr conn, remote_nonnull_domain_snapshot snapshot); +static virDomainSnapshotPtr get_nonnull_domain_snapshot (virDomainPtr conn, remote_nonnull_domain_snapshot snapshot);

s/conn/domain/

ACK

Thanks, I fixed this minor problem and pushed. -- Chris Lalancette

On 04/23/2010 02:06 PM, Matthias Bolte wrote:

2010/4/23 Chris Lalancette <clalance@redhat.com>:

...

Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/conf/domain_conf.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6dc26c3..139712a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6614,8 +6614,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, }

if (!xmlStrEqual(root->name, BAD_CAST "domainsnapshot")) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("incorrect root element")); + virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("domainsnapshot")); goto cleanup; }

Could we have a bit more verbose error message here, like "unknown root element for domain snapshot"?

Well, we could go one of two ways. The text that automatically comes out from VIR_ERR_XML_ERROR is: XML description for %s is not well formed or invalid Where the %s would be replaced by "domainsnapshot" above. So we could either go with this patch, or we could do something more along the lines of: virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unknown root element for domain snapshot")); I personally like this patch as it is, because it's clearly not an internal error; it's an error in the data the user passed in. But I don't really care too much either way. -- Chris Lalancette

2010/4/23 Chris Lalancette <clalance@redhat.com>:

We were freeing the virDomainSnapshotDefPtr, but not the virDomainSnapshotObjPtr in virDomainSnapshotObjFree.

Signed-off-by: Chris Lalancette <clalance@redhat.com> ---  src/conf/domain_conf.c |    1 +  1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 139712a..3ad24cb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6754,6 +6754,7 @@ static void virDomainSnapshotObjFree(virDomainSnapshotObjPtr snapshot)     VIR_DEBUG("obj=%p", snapshot);

    virDomainSnapshotDefFree(snapshot->def); +    VIR_FREE(snapshot);  }

 int virDomainSnapshotObjUnref(virDomainSnapshotObjPtr snapshot) -- 1.6.6.1

ACK. Matthias

On 04/23/2010 02:10 PM, Matthias Bolte wrote:

2010/4/23 Chris Lalancette <clalance@redhat.com>:

...

While doing some testing of the snapshot code I noticed that if qemuDomainSnapshotLoad failed, it would print a NULL as part of the error. That's not desirable, so leave the full_path variable around until after we are done printing errors.

Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2f889d9..7314328 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1388,9 +1388,9 @@ static void qemuDomainSnapshotLoad(void *payload, }

ret = virFileReadAll(fullpath, 1024*1024*1, &xmlStr); - VIR_FREE(fullpath); if (ret < 0) { /* Nothing we can do here, skip this one */ + VIR_FREE(fullpath); VIR_ERROR("Failed to read snapshot file %s: %s", fullpath, virStrerror(errno, ebuf, sizeof(ebuf)));

You still free fullpath here before using it.

...

continue; @@ -1400,12 +1400,14 @@ static void qemuDomainSnapshotLoad(void *payload, if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR("Failed to parse snapshot XML from file '%s'", fullpath); + VIR_FREE(fullpath);

Here it's in the correct order.

Oops, thanks. Since it's a fairly minor patch, I fixed it with your correction and pushed. -- Chris Lalancette