12:27 p.m.
Hello,
This series of patches fixes up a number of problems discovered
while testing the snapshot code. Descriptions of the fixes are
in the individual patches. Please review.
Thanks,
Chris Lalancette
12:27 p.m.
New subject: [libvirt] [PATCH 1/8] Fix up the locking in the snapshot code.
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(a)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);
--
1.6.6.1
7:26 a.m.
New subject: [libvirt] [PATCH 1/8] Fix up the locking in the snapshot code.
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(a)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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:49 a.m.
New subject: [libvirt] [PATCH 1/8] Fix up the locking in the snapshot code.
On 04/27/2010 08:26 AM, Daniel Veillard wrote:
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(a)redhat.com>
<snip>
ACK, looks fine to me,
Thanks, I pushed this now.
--
Chris Lalancette
12:27 p.m.
New subject: [libvirt] [PATCH 2/8] Fix a memory leak in the snapshot code in libvirtd.
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(a)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);
static void make_nonnull_domain (remote_nonnull_domain *dom_dst, virDomainPtr dom_src);
static void make_nonnull_network (remote_nonnull_network *net_dst, virNetworkPtr
net_src);
static void make_nonnull_interface (remote_nonnull_interface *interface_dst,
virInterfacePtr interface_src);
@@ -5905,25 +5905,34 @@ remoteDispatchDomainSnapshotDumpXml (struct qemud_server *server
ATTRIBUTE_UNUSE
remote_domain_snapshot_dump_xml_args *args,
remote_domain_snapshot_dump_xml_ret *ret)
{
- virDomainSnapshotPtr snapshot;
+ virDomainPtr domain = NULL;
+ virDomainSnapshotPtr snapshot = NULL;
+ int rc = -1;
- snapshot = get_nonnull_domain_snapshot(conn, args->snap);
- if (snapshot == NULL) {
- remoteDispatchConnError(rerr, conn);
- return -1;
- }
+ domain = get_nonnull_domain(conn, args->snap.domain);
+ if (domain == NULL)
+ goto cleanup;
+
+ snapshot = get_nonnull_domain_snapshot(domain, args->snap);
+ if (snapshot == NULL)
+ goto cleanup;
/* remoteDispatchClientRequest will free this. */
ret->xml = virDomainSnapshotGetXMLDesc(snapshot, args->flags);
- if (!ret->xml) {
+ if (!ret->xml)
+ goto cleanup;
+
+ rc = 0;
+
+cleanup:
+ if (snapshot)
virDomainSnapshotFree(snapshot);
+ if (domain)
+ virDomainFree(domain);
+ if (rc < 0)
remoteDispatchConnError(rerr, conn);
- return -1;
- }
- virDomainSnapshotFree(snapshot);
-
- return 0;
+ return rc;
}
static int
@@ -6108,23 +6117,32 @@ remoteDispatchDomainRevertToSnapshot (struct qemud_server *server
ATTRIBUTE_UNUS
remote_domain_revert_to_snapshot_args *args,
void *ret ATTRIBUTE_UNUSED)
{
- virDomainSnapshotPtr snapshot;
+ virDomainPtr domain = NULL;
+ virDomainSnapshotPtr snapshot = NULL;
+ int rc = -1;
- snapshot = get_nonnull_domain_snapshot(conn, args->snap);
- if (snapshot == NULL) {
- remoteDispatchConnError(rerr, conn);
- return -1;
- }
+ domain = get_nonnull_domain(conn, args->snap.domain);
+ if (domain == NULL)
+ goto cleanup;
+
+ snapshot = get_nonnull_domain_snapshot(domain, args->snap);
+ if (snapshot == NULL)
+ goto cleanup;
- if (virDomainRevertToSnapshot(snapshot, args->flags) == -1) {
+ if (virDomainRevertToSnapshot(snapshot, args->flags) == -1)
+ goto cleanup;
+
+ rc = 0;
+
+cleanup:
+ if (snapshot)
virDomainSnapshotFree(snapshot);
+ if (domain)
+ virDomainFree(domain);
+ if (rc < 0)
remoteDispatchConnError(rerr, conn);
- return -1;
- }
- virDomainSnapshotFree(snapshot);
-
- return 0;
+ return rc;
}
static int
@@ -6136,23 +6154,32 @@ remoteDispatchDomainSnapshotDelete (struct qemud_server *server
ATTRIBUTE_UNUSED
remote_domain_snapshot_delete_args *args,
void *ret ATTRIBUTE_UNUSED)
{
- virDomainSnapshotPtr snapshot;
+ virDomainPtr domain = NULL;
+ virDomainSnapshotPtr snapshot = NULL;
+ int rc = -1;
- snapshot = get_nonnull_domain_snapshot(conn, args->snap);
- if (snapshot == NULL) {
- remoteDispatchConnError(rerr, conn);
- return -1;
- }
+ domain = get_nonnull_domain(conn, args->snap.domain);
+ if (domain == NULL)
+ goto cleanup;
+
+ snapshot = get_nonnull_domain_snapshot(domain, args->snap);
+ if (snapshot == NULL)
+ goto cleanup;
- if (virDomainSnapshotDelete(snapshot, args->flags) == -1) {
+ if (virDomainSnapshotDelete(snapshot, args->flags) == -1)
+ goto cleanup;
+
+ rc = 0;
+
+cleanup:
+ if (snapshot)
virDomainSnapshotFree(snapshot);
+ if (domain)
+ virDomainFree(domain);
+ if (rc < 0)
remoteDispatchConnError(rerr, conn);
- return -1;
- }
- virDomainSnapshotFree(snapshot);
-
- return 0;
+ return rc;
}
@@ -6466,12 +6493,8 @@ get_nonnull_nwfilter (virConnectPtr conn, remote_nonnull_nwfilter
nwfilter)
}
static virDomainSnapshotPtr
-get_nonnull_domain_snapshot (virConnectPtr conn, remote_nonnull_domain_snapshot
snapshot)
+get_nonnull_domain_snapshot (virDomainPtr domain, remote_nonnull_domain_snapshot
snapshot)
{
- virDomainPtr domain;
- domain = get_nonnull_domain(conn, snapshot.domain);
- if (domain == NULL)
- return NULL;
return virGetDomainSnapshot(domain, snapshot.name);
}
--
1.6.6.1
12:57 p.m.
New subject: [libvirt] [PATCH 2/8] Fix a memory leak in the snapshot code in libvirtd.
2010/4/23 Chris Lalancette <clalance(a)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(a)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
Matthias
1:20 p.m.
New subject: [libvirt] [PATCH 2/8] Fix a memory leak in the snapshot code in libvirtd.
On 04/23/2010 01:57 PM, Matthias Bolte wrote:
2010/4/23 Chris Lalancette <clalance(a)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(a)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
12:27 p.m.
New subject: [libvirt] [PATCH 3/8] Make virDomainSnapshotObjListDeinit static.
Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
---
src/conf/domain_conf.c | 3 ++-
src/conf/domain_conf.h | 1 -
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 43d74cf..6dc26c3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -733,6 +733,7 @@ void virDomainDefFree(virDomainDefPtr def)
#ifndef PROXY
+static void virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots);
static void virDomainObjFree(virDomainObjPtr dom)
{
if (!dom)
@@ -6811,7 +6812,7 @@ static void virDomainSnapshotObjListDeallocator(void *payload,
virDomainSnapshotObjUnref(obj);
}
-void virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots)
+static void virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots)
{
if (snapshots->objs)
virHashFree(snapshots->objs, virDomainSnapshotObjListDeallocator);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0e5a6a6..de0c99a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -783,7 +783,6 @@ virDomainSnapshotObjPtr
virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr s
const virDomainSnapshotDefPtr def);
int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr objs);
-void virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr objs);
int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots,
char **const names, int maxnames);
int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots);
--
1.6.6.1
12:59 p.m.
New subject: [libvirt] [PATCH 3/8] Make virDomainSnapshotObjListDeinit static.
2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
---
src/conf/domain_conf.c | 3 ++-
src/conf/domain_conf.h | 1 -
2 files changed, 2 insertions(+), 2 deletions(-)
ACK.
Matthias
1:21 p.m.
New subject: [libvirt] [PATCH 3/8] Make virDomainSnapshotObjListDeinit static.
On 04/23/2010 01:59 PM, Matthias Bolte wrote:
2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
> Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
> ---
> src/conf/domain_conf.c | 3 ++-
> src/conf/domain_conf.h | 1 -
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
ACK.
Thanks, pushed.
--
Chris Lalancette
12:27 p.m.
New subject: [libvirt] [PATCH 4/8] Fix up the error message if we can't parse the snapshot XML.
Signed-off-by: Chris Lalancette <clalance(a)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;
}
--
1.6.6.1
1:06 p.m.
New subject: [libvirt] [PATCH 4/8] Fix up the error message if we can't parse the snapshot XML.
2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
Signed-off-by: Chris Lalancette <clalance(a)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"?
Matthias
9:52 a.m.
New subject: [libvirt] [PATCH 4/8] Fix up the error message if we can't parse the snapshot XML.
On 04/23/2010 02:06 PM, Matthias Bolte wrote:
2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
> Signed-off-by: Chris Lalancette <clalance(a)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
1:08 p.m.
New subject: [libvirt] [PATCH 4/8] Fix up the error message if we can't parse the snapshot XML.
2010/4/27 Chris Lalancette <clalance(a)redhat.com>:
On 04/23/2010 02:06 PM, Matthias Bolte wrote:
> 2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
>> Signed-off-by: Chris Lalancette <clalance(a)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
Ah, sorry. I missed the fact that VIR_ERR_XML_ERROR results in this
format string. I'm fine with the original patch then.
ACK.
If you grep the codebase for VIR_ERR_XML_ERROR then you'll find that
in most cases VIR_ERR_XML_ERROR is used in the wrong way, resulting in
a broken error message:
virInterfaceReportError(VIR_ERR_XML_ERROR,
"%s", _("bridge interface
misses the bridge element"));
Just reporting that the XML in not well formed (the string for
VIR_ERR_XML_ERROR implies this usage) is not that helpful. So we
include the relevant details in the error message, ignoring that the
string associated with VIR_ERR_XML_ERROR indicates a different usage.
Maybe we should unify the structure of the error code strings to a
format that expects the additional error message as a more detailed
description of the error. For example VIR_ERR_NO_DOMAIN or
VIR_ERR_INVALID_MAC or VIR_ERR_AUTH_FAILED already have this format.
Matthias
1:11 p.m.
New subject: [libvirt] [PATCH 4/8] Fix up the error message if we can't parse the snapshot XML.
On 04/27/2010 02:08 PM, Matthias Bolte wrote:
> XML description for %s is not well formed or invalid
>
Ah, sorry. I missed the fact that VIR_ERR_XML_ERROR results in this
format string. I'm fine with the original patch then.
ACK.
If you grep the codebase for VIR_ERR_XML_ERROR then you'll find that
in most cases VIR_ERR_XML_ERROR is used in the wrong way, resulting in
a broken error message:
virInterfaceReportError(VIR_ERR_XML_ERROR,
"%s", _("bridge interface
misses the bridge element"));
Just reporting that the XML in not well formed (the string for
VIR_ERR_XML_ERROR implies this usage) is not that helpful. So we
include the relevant details in the error message, ignoring that the
string associated with VIR_ERR_XML_ERROR indicates a different usage.
Maybe we should unify the structure of the error code strings to a
format that expects the additional error message as a more detailed
description of the error. For example VIR_ERR_NO_DOMAIN or
VIR_ERR_INVALID_MAC or VIR_ERR_AUTH_FAILED already have this format.
Yeah, I've noticed this as well. I think your suggestion is a good idea.
I've pushed this patch for now, and hopefully we can do a better job in a
follow-up patch.
--
Chris Lalancette
12:27 p.m.
New subject: [libvirt] [PATCH 5/8] Fix virDomainSnapshotObjFree memory leak.
We were freeing the virDomainSnapshotDefPtr, but not
the virDomainSnapshotObjPtr in virDomainSnapshotObjFree.
Signed-off-by: Chris Lalancette <clalance(a)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
1:08 p.m.
New subject: [libvirt] [PATCH 5/8] Fix virDomainSnapshotObjFree memory leak.
2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
We were freeing the virDomainSnapshotDefPtr, but not
the virDomainSnapshotObjPtr in virDomainSnapshotObjFree.
Signed-off-by: Chris Lalancette <clalance(a)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
1:21 p.m.
New subject: [libvirt] [PATCH 5/8] Fix virDomainSnapshotObjFree memory leak.
On 04/23/2010 02:08 PM, Matthias Bolte wrote:
2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
> We were freeing the virDomainSnapshotDefPtr, but not
> the virDomainSnapshotObjPtr in virDomainSnapshotObjFree.
>
> Signed-off-by: Chris Lalancette <clalance(a)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.
Thanks, pushed.
--
Chris Lalancette
12:27 p.m.
New subject: [libvirt] [PATCH 6/8] Fix printing of pathnames on error in qemuDomainSnapshotLoad.
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(a)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)));
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);
VIR_FREE(xmlStr);
continue;
}
virDomainSnapshotAssignDef(&vm->snapshots, def);
+ VIR_FREE(fullpath);
VIR_FREE(xmlStr);
}
--
1.6.6.1
1:10 p.m.
New subject: [libvirt] [PATCH 6/8] Fix printing of pathnames on error in qemuDomainSnapshotLoad.
2010/4/23 Chris Lalancette <clalance(a)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(a)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.
Matthias
1:23 p.m.
New subject: [libvirt] [PATCH 6/8] Fix printing of pathnames on error in qemuDomainSnapshotLoad.
On 04/23/2010 02:10 PM, Matthias Bolte wrote:
2010/4/23 Chris Lalancette <clalance(a)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(a)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
12:27 p.m.
New subject: [libvirt] [PATCH 7/8] Report better error if qemuSnapshotIsAllowed failed.
Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
---
src/qemu/qemu_driver.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7314328..b0c1877 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10673,8 +10673,8 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
(!vm->def->disks[i]->driverType ||
STRNEQ(vm->def->disks[i]->driverType, "qcow2"))) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
- _("Disk device '%s' does not support
snapshotting"),
- vm->def->disks[i]->info.alias);
+ _("Disk '%s' does not support
snapshotting"),
+ vm->def->disks[i]->src);
return 0;
}
}
--
1.6.6.1
12:27 p.m.
New subject: [libvirt] [PATCH 8/8] Fix memory leak in virsh snapshot-list.
We were forgetting to release the memory allocated by
virDomainSnapshotListNames. Free the memory properly.
Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
---
tools/virsh.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 93a9f45..04b47a1 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -8345,7 +8345,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
int ret = FALSE;
int numsnaps;
char **names = NULL;
- int actual;
+ int actual = 0;
int i;
xmlDocPtr xml = NULL;
xmlXPathContextPtr ctxt = NULL;
@@ -8432,6 +8432,8 @@ cleanup:
if (xml)
xmlFreeDoc(xml);
VIR_FREE(doc);
+ for (i = 0; i < actual; i++)
+ VIR_FREE(names[i]);
VIR_FREE(names);
if (dom)
virDomainFree(dom);
--
1.6.6.1
1:12 p.m.
New subject: [libvirt] [PATCH 8/8] Fix memory leak in virsh snapshot-list.
2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
We were forgetting to release the memory allocated by
virDomainSnapshotListNames. Free the memory properly.
Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
---
tools/virsh.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
ACK.
Matthias
1:23 p.m.
New subject: [libvirt] [PATCH 8/8] Fix memory leak in virsh snapshot-list.
On 04/23/2010 02:12 PM, Matthias Bolte wrote:
2010/4/23 Chris Lalancette <clalance(a)redhat.com>:
> We were forgetting to release the memory allocated by
> virDomainSnapshotListNames. Free the memory properly.
>
> Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
> ---
> tools/virsh.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
ACK.
Thanks, pushed.
--
Chris Lalancette
5323
days inactive
5327
days old
24 comments
3 participants
participants (3)
-
Chris Lalancette -
Daniel Veillard -
Matthias Bolte