[libvirt] [PATCH 0/4] Fix segfault while redefining snapshots and allow redefinition of external snapshots
7:38 a.m.
This series fixes two issues while redefining snapshots:
1) Segfault when disk alignment of the new snapshot failed (patch 3)
2) Failure to redefine external snapshots. A check was missing to align disks for external
snapshots. (patch 4)
Peter Krempa (4):
snapshot: conf: Make virDomainSnapshotIsExternal more reusable
snapshot: qemu: Separate logic blocks with newlines
snapshot: qemu: Fix segfault and vanishing snapshots when redefining
snapshot: qemu: Allow redefinition of external snapshots
src/conf/snapshot_conf.c | 14 ++++++++----
src/conf/snapshot_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 57 +++++++++++++++++++++++++++++++++++-------------
4 files changed, 54 insertions(+), 19 deletions(-)
--
1.8.0.2
7:38 a.m.
New subject: [libvirt] [PATCH 1/4] snapshot: conf: Make virDomainSnapshotIsExternal more reusable
Allow to use definition objects with this predicate function.
---
src/conf/snapshot_conf.c | 14 ++++++++++----
src/conf/snapshot_conf.h | 1 +
src/libvirt_private.syms | 1 +
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 201c586..0c5b005 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1058,17 +1058,23 @@ cleanup:
bool
-virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap)
+virDomainSnapshotDefIsExternal(virDomainSnapshotDefPtr def)
{
int i;
- if (snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
return true;
- for (i = 0; i < snap->def->ndisks; i++) {
- if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ for (i = 0; i < def->ndisks; i++) {
+ if (def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
return true;
}
return false;
}
+
+bool
+virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap)
+{
+ return virDomainSnapshotDefIsExternal(snap->def);
+}
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index b0f8760..f1d5995 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -166,6 +166,7 @@ int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots,
virDomainSnapshotPtr **snaps,
unsigned int flags);
+bool virDomainSnapshotDefIsExternal(virDomainSnapshotDefPtr def);
bool virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap);
VIR_ENUM_DECL(virDomainSnapshotLocation)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 497d5d3..d063382 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1097,6 +1097,7 @@ virDomainSnapshotAlignDisks;
virDomainSnapshotAssignDef;
virDomainSnapshotDefFormat;
virDomainSnapshotDefFree;
+virDomainSnapshotDefIsExternal;
virDomainSnapshotDefParseString;
virDomainSnapshotDropParent;
virDomainSnapshotFindByName;
--
1.8.0.2
12:17 p.m.
New subject: [libvirt] [PATCH 1/4] snapshot: conf: Make virDomainSnapshotIsExternal more reusable
On 01/03/2013 06:38 AM, Peter Krempa wrote:
Allow to use definition objects with this predicate function.
---
src/conf/snapshot_conf.c | 14 ++++++++++----
src/conf/snapshot_conf.h | 1 +
src/libvirt_private.syms | 1 +
3 files changed, 12 insertions(+), 4 deletions(-)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:38 a.m.
New subject: [libvirt] [PATCH 2/4] snapshot: qemu: Separate logic blocks with newlines
---
src/qemu/qemu_driver.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b8bb36b..0883a56 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11365,6 +11365,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
goto cleanup;
}
+
if (def->dom &&
memcmp(def->dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
virReportError(VIR_ERR_INVALID_ARG,
@@ -11372,6 +11373,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
def->name, uuidstr);
goto cleanup;
}
+
other = virDomainSnapshotFindByName(vm->snapshots, def->name);
if (other) {
if ((other->def->state == VIR_DOMAIN_RUNNING ||
@@ -11384,6 +11386,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
def->name);
goto cleanup;
}
+
if ((other->def->state == VIR_DOMAIN_DISK_SNAPSHOT) !=
(def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
virReportError(VIR_ERR_INVALID_ARG,
@@ -11392,6 +11395,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
def->name);
goto cleanup;
}
+
if (other->def->dom) {
if (def->dom) {
if (!virDomainDefCheckABIStability(other->def->dom,
@@ -11403,10 +11407,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
other->def->dom = NULL;
}
}
+
if (other == vm->current_snapshot) {
update_current = true;
vm->current_snapshot = NULL;
}
+
/* Drop and rebuild the parent relationship, but keep all
* child relations by reusing snap. */
virDomainSnapshotDropParent(other);
--
1.8.0.2
12:18 p.m.
New subject: [libvirt] [PATCH 2/4] snapshot: qemu: Separate logic blocks with newlines
On 01/03/2013 06:38 AM, Peter Krempa wrote:
---
src/qemu/qemu_driver.c | 6 ++++++
1 file changed, 6 insertions(+)
Cosmetic only. ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:38 a.m.
New subject: [libvirt] [PATCH 3/4] snapshot: qemu: Fix segfault and vanishing snapshots when redefining
When the disk alignment check done while redefining an existing snapshot
failed, the qemu driver attempted to free the existing snapshot. As in
the cleanup path the definition of the snapshot wasn't assigned, the
cleanup code dereferenced a NULL pointer.
This patch changes the behavior on error paths while redefining snapshot
in two ways:
1) On failure, modifications done on the snapshot definiton object are
rolled back.
2) The previous definition of the data isn't freed until it's certain it
won't be needed any more.
This change avoids the segfault and additionaly the snapshot doesn't
vanish if re-definiton fails for some reason.
---
src/qemu/qemu_driver.c | 51 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0883a56..4c7558d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11408,6 +11408,24 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
}
}
+ if (def->dom) {
+ if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
+ def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+ align_match = false;
+ }
+
+ if (virDomainSnapshotAlignDisks(def, align_location,
+ align_match) < 0) {
+ /* revert stealing of the snapshot domain definition */
+ if (def->dom && !other->def->dom) {
+ other->def->dom = def->dom;
+ def->dom = NULL;
+ }
+ goto cleanup;
+ }
+ }
+
if (other == vm->current_snapshot) {
update_current = true;
vm->current_snapshot = NULL;
@@ -11417,18 +11435,20 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
* child relations by reusing snap. */
virDomainSnapshotDropParent(other);
virDomainSnapshotDefFree(other->def);
- other->def = NULL;
+ other->def = def;
+ def = NULL;
snap = other;
- }
- if (def->dom) {
- if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
- def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
- align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
+ } else {
+ if (def->dom) {
+ if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
+ def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+ align_match = false;
+ }
+ if (virDomainSnapshotAlignDisks(def, align_location,
+ align_match) < 0)
+ goto cleanup;
}
- if (virDomainSnapshotAlignDisks(def, align_location,
- align_match) < 0)
- goto cleanup;
}
} else {
/* Easiest way to clone inactive portion of vm->def is via
@@ -11463,11 +11483,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
goto cleanup;
}
- if (snap)
- snap->def = def;
- else if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
- goto cleanup;
- def = NULL;
+ if (!snap) {
+ if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
+ goto cleanup;
+
+ def = NULL;
+ }
if (update_current)
snap->def->current = true;
--
1.8.0.2
12:26 p.m.
New subject: [libvirt] [PATCH 3/4] snapshot: qemu: Fix segfault and vanishing snapshots when redefining
On 01/03/2013 06:38 AM, Peter Krempa wrote:
When the disk alignment check done while redefining an existing
snapshot
failed, the qemu driver attempted to free the existing snapshot. As in
the cleanup path the definition of the snapshot wasn't assigned, the
cleanup code dereferenced a NULL pointer.
This patch changes the behavior on error paths while redefining snapshot
in two ways:
1) On failure, modifications done on the snapshot definiton object are
s/definiton/definition/
rolled back.
2) The previous definition of the data isn't freed until it's certain it
won't be needed any more.
This change avoids the segfault and additionaly the snapshot doesn't
s/additionaly/additionally/
vanish if re-definiton fails for some reason.
s/re-definiton/redefinition/
---
src/qemu/qemu_driver.c | 51 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 15 deletions(-)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7 p.m.
New subject: [libvirt] [PATCH 3/4] snapshot: qemu: Fix segfault and vanishing snapshots when redefining
On 01/04/13 19:26, Eric Blake wrote:
On 01/03/2013 06:38 AM, Peter Krempa wrote:
> When the disk alignment check done while redefining an existing snapshot
> failed, the qemu driver attempted to free the existing snapshot. As in
> the cleanup path the definition of the snapshot wasn't assigned, the
> cleanup code dereferenced a NULL pointer.
>
> This patch changes the behavior on error paths while redefining snapshot
> in two ways:
>
> 1) On failure, modifications done on the snapshot definiton object are
s/definiton/definition/
> rolled back.
>
> 2) The previous definition of the data isn't freed until it's certain it
> won't be needed any more.
>
> This change avoids the segfault and additionaly the snapshot doesn't
s/additionaly/additionally/
> vanish if re-definiton fails for some reason.
s/re-definiton/redefinition/
> ---
> src/qemu/qemu_driver.c | 51 +++++++++++++++++++++ ++++++++++++++---------------
> 1 file changed, 36 insertions(+), 15 deletions(-)
>
The amount typos in my patches is really embarrassing. I should get
spell checking support for my editor :/.
ACK.
Thanks for the review.
Peter
7:38 a.m.
New subject: [libvirt] [PATCH 4/4] snapshot: qemu: Allow redefinition of external snapshots
A redefinition of an external inactive snapshot/checkpoint wasn't
possible without this change.
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4c7558d..3a52b47 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11410,7 +11410,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (def->dom) {
if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
- def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ virDomainSnapshotDefIsExternal(def)) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
}
--
1.8.0.2
12:26 p.m.
New subject: [libvirt] [PATCH 4/4] snapshot: qemu: Allow redefinition of external snapshots
On 01/03/2013 06:38 AM, Peter Krempa wrote:
A redefinition of an external inactive snapshot/checkpoint
wasn't
possible without this change.
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4c7558d..3a52b47 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11410,7 +11410,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (def->dom) {
if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
- def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ virDomainSnapshotDefIsExternal(def)) {
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:47 a.m.
New subject: [libvirt] [PATCH 4/4] snapshot: qemu: Allow redefinition of external snapshots
On 01/04/13 19:26, Eric Blake wrote:
On 01/03/2013 06:38 AM, Peter Krempa wrote:
> A redefinition of an external inactive snapshot/checkpoint wasn't
> possible without this change.
> ---
ACK.
I've pushed the series. Thanks for the review.
Peter
4374
days inactive
4376
days old
10 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa