7:36 a.m.
The external snapshot code mistakenly used non-"migratable" XMLs
in the save image which caused problems with restoring them
via the workaround way of doing virDomainRestore().
This series adds workaround to add compatibility with older snapshots
and fixes the problem in the snapshot code.
Unfortunately, this is not testable with unit tests. I'll try to
do a autotest virt-test test case for this particular issue.
Peter Krempa (2):
qemu: Fix checking of ABI stability when restoring external
checkpoints
qemu: Use "migratable" XML definition when doing external checkpoints
src/qemu/qemu_driver.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
--
1.8.3.2
7:36 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix checking of ABI stability when restoring external checkpoints
External checkpoints have a bug in the implementation where they use the
normal definition instead of the "migratable" one. This causes errors
when the snapshot is being reverted using the workaround method via
qemuDomainRestoreFlags() with a custom XML. This issue was introduced
when commit 07966f6a8b5ccb5bb4c716b25deb8ba2e572cc67 changed the code to
compare "migratable" XMLs from the user as we should have used
migratable in the image too.
This patch adds a compatibility layer, so that fixing the snapshot code
won't make existing snapshots fail to load.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1008340
---
src/qemu/qemu_driver.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae1948f..22497f0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5251,14 +5251,30 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
goto error;
newdef = qemuDomainDefCopy(driver, def2, VIR_DOMAIN_XML_MIGRATABLE);
- virDomainDefFree(def2);
- if (!newdef)
+ if (!newdef) {
+ virDomainDefFree(def2);
goto error;
+ }
if (!virDomainDefCheckABIStability(def, newdef)) {
virDomainDefFree(newdef);
- goto error;
+ virResetLastError();
+
+ /* Due to a bug in external snapshot creation code, the XML saved
+ * in the save image was not a migratable XML. To ensure backwards
+ * compatibility with the change of the saved XML type, we need
+ * to check the ABI compatibility against the user provided XML if
+ * the check against the migratable XML fails. */
+ if (!virDomainDefCheckABIStability(def, def2)) {
+ virDomainDefFree(def2);
+ goto error;
+ }
+
+ /* use the user provided XML */
+ newdef = def2;
+ def2 = NULL;
}
+
virDomainDefFree(def);
def = newdef;
}
--
1.8.3.2
11:57 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix checking of ABI stability when restoring external checkpoints
On 09/16/2013 06:36 AM, Peter Krempa wrote:
External checkpoints have a bug in the implementation where they use
the
normal definition instead of the "migratable" one. This causes errors
when the snapshot is being reverted using the workaround method via
qemuDomainRestoreFlags() with a custom XML. This issue was introduced
when commit 07966f6a8b5ccb5bb4c716b25deb8ba2e572cc67 changed the code to
compare "migratable" XMLs from the user as we should have used
migratable in the image too.
This patch adds a compatibility layer, so that fixing the snapshot code
won't make existing snapshots fail to load.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1008340
---
src/qemu/qemu_driver.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
Uggh, but makes sense.
if (!virDomainDefCheckABIStability(def, newdef)) {
virDomainDefFree(newdef);
- goto error;
+ virResetLastError();
+
+ /* Due to a bug in external snapshot creation code, the XML saved
a bug in older versions of external snapshot creation code,
ACK with that comment tweak (and maybe even call out that external
snapshots created prior to 1.1.3 have the bug).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:56 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix checking of ABI stability when restoring external checkpoints
On 09/16/13 18:57, Eric Blake wrote:
On 09/16/2013 06:36 AM, Peter Krempa wrote:
> External checkpoints have a bug in the implementation where they use the
> normal definition instead of the "migratable" one. This causes errors
> when the snapshot is being reverted using the workaround method via
> qemuDomainRestoreFlags() with a custom XML. This issue was introduced
> when commit 07966f6a8b5ccb5bb4c716b25deb8ba2e572cc67 changed the code to
> compare "migratable" XMLs from the user as we should have used
> migratable in the image too.
>
> This patch adds a compatibility layer, so that fixing the snapshot code
> won't make existing snapshots fail to load.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1008340
> ---
> src/qemu/qemu_driver.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
Uggh, but makes sense.
>
> if (!virDomainDefCheckABIStability(def, newdef)) {
> virDomainDefFree(newdef);
> - goto error;
> + virResetLastError();
> +
> + /* Due to a bug in external snapshot creation code, the XML saved
a bug in older versions of external snapshot creation code,
ACK with that comment tweak (and maybe even call out that external
snapshots created prior to 1.1.3 have the bug).
I've fixed the comment and mentioned the broken versions and pushed.
Thanks.
PEter
3:51 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix checking of ABI stability when restoring external checkpoints
On 09/16/2013 02:36 PM, Peter Krempa wrote:
External checkpoints have a bug in the implementation where they use
the
normal definition instead of the "migratable" one. This causes errors
when the snapshot is being reverted using the workaround method via
qemuDomainRestoreFlags() with a custom XML. This issue was introduced
when commit 07966f6a8b5ccb5bb4c716b25deb8ba2e572cc67 changed the code to
compare "migratable" XMLs from the user as we should have used
migratable in the image too.
This patch adds a compatibility layer, so that fixing the snapshot code
won't make existing snapshots fail to load.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1008340
---
src/qemu/qemu_driver.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae1948f..22497f0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5251,14 +5251,30 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
goto error;
newdef = qemuDomainDefCopy(driver, def2, VIR_DOMAIN_XML_MIGRATABLE);
- virDomainDefFree(def2);
You removed the freeing of def2 here and only free it on errors.
- if (!newdef)
+ if (!newdef) {
+ virDomainDefFree(def2);
goto error;
+ }
if (!virDomainDefCheckABIStability(def, newdef)) {
virDomainDefFree(newdef);
- goto error;
+ virResetLastError();
+
+ /* Due to a bug in external snapshot creation code, the XML saved
+ * in the save image was not a migratable XML. To ensure backwards
+ * compatibility with the change of the saved XML type, we need
+ * to check the ABI compatibility against the user provided XML if
+ * the check against the migratable XML fails. */
+ if (!virDomainDefCheckABIStability(def, def2)) {
+ virDomainDefFree(def2);
+ goto error;
+ }
+
+ /* use the user provided XML */
+ newdef = def2;
+ def2 = NULL;
def2 is not used after this assignment.
}
+
virDomainDefFree(def);
def = newdef;
}
Jan
7:36 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use "migratable" XML definition when doing external checkpoints
In the original implementation of external checkpoints I've mistakenly
used the live definition to be stored in the save image. The normal
approach is to use the "migratable" definition. This was discovered when
commit 07966f6a8b5ccb5bb4c716b25deb8ba2e572cc67 changed the behavior to
use a converted XML from the user to do the compatibility check to fix
problem when using the regular machine saving.
As the previous patch added a compatibility layer, we can now change the
type of the XML in the image.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1008340
---
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 22497f0..0b2ff7d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12197,7 +12197,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
JOB_MASK(QEMU_JOB_SUSPEND) |
JOB_MASK(QEMU_JOB_MIGRATION_OP));
- if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, false)))
+ if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, true)))
goto endjob;
if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file,
--
1.8.3.2
11:58 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use "migratable" XML definition when doing external checkpoints
On 09/16/2013 06:36 AM, Peter Krempa wrote:
In the original implementation of external checkpoints I've
mistakenly
used the live definition to be stored in the save image. The normal
approach is to use the "migratable" definition. This was discovered when
commit 07966f6a8b5ccb5bb4c716b25deb8ba2e572cc67 changed the behavior to
use a converted XML from the user to do the compatibility check to fix
problem when using the regular machine saving.
As the previous patch added a compatibility layer, we can now change the
type of the XML in the image.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1008340
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:57 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use "migratable" XML definition when doing external checkpoints
On 09/16/13 18:58, Eric Blake wrote:
On 09/16/2013 06:36 AM, Peter Krempa wrote:
> In the original implementation of external checkpoints I've mistakenly
> used the live definition to be stored in the save image. The normal
> approach is to use the "migratable" definition. This was discovered when
> commit 07966f6a8b5ccb5bb4c716b25deb8ba2e572cc67 changed the behavior to
> use a converted XML from the user to do the compatibility check to fix
> problem when using the regular machine saving.
>
> As the previous patch added a compatibility layer, we can now change the
> type of the XML in the image.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1008340
> ---
> src/qemu/qemu_driver.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
Pushed; thanks.
Peter
4063
days inactive
4064
days old
7 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa