[libvirt] [PATCH 0/2] Fix reverting of external snapshots with custom XML
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
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
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
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
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
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
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
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
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa