[libvirt PATCH v2 0/6] external snapshot revert fixes
This fixes reverting external snapshots to not error out in cases where it should work and makes it correctly load the memory state when reverting to snapshot of running VM. Last patch is not part of the fix, it is alternative solution to remove the capability as we are close to releasing libvirt 9.7.0 in case the code fixing reverting snapshots is considered too complex. This v2 limits the impact only to snapshot code and no longer changes anything in qemu_saveimage except for exporting one function and one enum. Pavel Hrdina (6): qemu_snapshot: fix reverting external snapshot when not all disks are included qemu_saveimage: export virQEMUSaveFormat enum qemu_saveimage: export qemuSaveImageGetCompressionCommand qemu_snapshot: correctly load the saved memory state file NEWS: document support for reverting external snapshots Revert "capabilities: report full external snapshot support" NEWS.rst | 8 ++ docs/formatcaps.rst | 7 - src/conf/capabilities.c | 1 - src/conf/capabilities.h | 1 - src/conf/schemas/capability.rng | 5 - src/qemu/qemu_capabilities.c | 1 - src/qemu/qemu_saveimage.c | 19 +-- src/qemu/qemu_saveimage.h | 20 +++ src/qemu/qemu_snapshot.c | 127 +++++++++++++++--- .../qemucaps2xmloutdata/caps.aarch64+hvf.xml | 1 - tests/qemucaps2xmloutdata/caps.aarch64.xml | 1 - tests/qemucaps2xmloutdata/caps.ppc.xml | 1 - tests/qemucaps2xmloutdata/caps.ppc64.xml | 1 - tests/qemucaps2xmloutdata/caps.riscv64.xml | 1 - tests/qemucaps2xmloutdata/caps.s390x.xml | 1 - tests/qemucaps2xmloutdata/caps.sparc.xml | 1 - tests/qemucaps2xmloutdata/caps.x86_64+hvf.xml | 1 - tests/qemucaps2xmloutdata/caps.x86_64.xml | 1 - 18 files changed, 138 insertions(+), 60 deletions(-) -- 2.41.0
We need to skip all disks that have snapshot type other than 'external'. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d943281e35..ff85d56572 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2058,6 +2058,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i]; virDomainDiskDef *domdisk = domdef->disks[i]; + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + if (qemuSnapshotPrepareDiskExternal(domdisk, snapdisk, active, false) < 0) return -1; } @@ -2098,6 +2101,9 @@ qemuSnapshotRevertExternalActive(virDomainObj *vm, return -1; for (i = 0; i < tmpsnapdef->ndisks; i++) { + if (tmpsnapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + if (qemuSnapshotDiskPrepareOne(snapctxt, vm->def->disks[i], tmpsnapdef->disks + i, @@ -2188,6 +2194,9 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm, for (i = 0; i < curdef->nrevertdisks; i++) { virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]); + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + if (virStorageSourceInit(snapdisk->src) < 0 || virStorageSourceUnlink(snapdisk->src) < 0) { VIR_WARN("Failed to remove snapshot image '%s'", @@ -2203,6 +2212,9 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm, for (i = 0; i < curdef->ndisks; i++) { virDomainSnapshotDiskDef *snapdisk = &(curdef->disks[i]); + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + if (virStorageSourceInit(snapdisk->src) < 0 || virStorageSourceUnlink(snapdisk->src) < 0) { VIR_WARN("Failed to remove snapshot image '%s'", -- 2.41.0
On Fri, Sep 01, 2023 at 10:32:12 +0200, Pavel Hrdina wrote:
We need to skip all disks that have snapshot type other than 'external'.
Since the commit message is vague on the specific problem details ...
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d943281e35..ff85d56572 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2058,6 +2058,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i]; virDomainDiskDef *domdisk = domdef->disks[i];
+ if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue;
... I remember you talking about the need to create overlays also for images which might have been excluded in given snapshot (the one you are reverting to) , but externally snapshotted in a later (still existing) one. In such case not creating the overlay here would still invalidate the overlay of the next snapshot. Is that right? I also remember that you mentioned that the actual problem is with empty cdroms, thus, did you rather want to use 'virStorageSourceIsEmpty' here?
On Fri, Sep 01, 2023 at 11:10:43AM +0200, Peter Krempa wrote:
On Fri, Sep 01, 2023 at 10:32:12 +0200, Pavel Hrdina wrote:
We need to skip all disks that have snapshot type other than 'external'.
Since the commit message is vague on the specific problem details ...
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d943281e35..ff85d56572 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2058,6 +2058,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i]; virDomainDiskDef *domdisk = domdef->disks[i];
+ if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue;
... I remember you talking about the need to create overlays also for images which might have been excluded in given snapshot (the one you are reverting to) , but externally snapshotted in a later (still existing) one.
In such case not creating the overlay here would still invalidate the overlay of the next snapshot. Is that right?
I also remember that you mentioned that the actual problem is with empty cdroms, thus, did you rather want to use 'virStorageSourceIsEmpty' here?
That is done in virDomainSnapshotAlignDisks() called right before the for loop in qemuSnapshotRevertExternalPrepare(), there we fill in the temporary snapshot definition with new overlays. From that moment the temporary snapshot definition contains all disks based on the VM definition. Looking at the code, when reverting to a snapshot we will always create new overlays for all disks including readonly disks, not sure if that is what we should do or no. Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 17 ----------------- src/qemu/qemu_saveimage.h | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 41310d6a9a..eca47171c2 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -37,23 +37,6 @@ VIR_LOG_INIT("qemu.qemu_saveimage"); -typedef enum { - QEMU_SAVE_FORMAT_RAW = 0, - QEMU_SAVE_FORMAT_GZIP = 1, - QEMU_SAVE_FORMAT_BZIP2 = 2, - /* - * Deprecated by xz and never used as part of a release - * QEMU_SAVE_FORMAT_LZMA - */ - QEMU_SAVE_FORMAT_XZ = 3, - QEMU_SAVE_FORMAT_LZOP = 4, - /* Note: add new members only at the end. - These values are used in the on-disk format. - Do not change or re-use numbers. */ - - QEMU_SAVE_FORMAT_LAST -} virQEMUSaveFormat; - VIR_ENUM_DECL(qemuSaveCompression); VIR_ENUM_IMPL(qemuSaveCompression, QEMU_SAVE_FORMAT_LAST, diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 30cf4b1ee0..6892e6764f 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -32,6 +32,23 @@ G_STATIC_ASSERT(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL)); +typedef enum { + QEMU_SAVE_FORMAT_RAW = 0, + QEMU_SAVE_FORMAT_GZIP = 1, + QEMU_SAVE_FORMAT_BZIP2 = 2, + /* + * Deprecated by xz and never used as part of a release + * QEMU_SAVE_FORMAT_LZMA + */ + QEMU_SAVE_FORMAT_XZ = 3, + QEMU_SAVE_FORMAT_LZOP = 4, + /* Note: add new members only at the end. + These values are used in the on-disk format. + Do not change or re-use numbers. */ + + QEMU_SAVE_FORMAT_LAST +} virQEMUSaveFormat; + typedef struct _virQEMUSaveHeader virQEMUSaveHeader; struct _virQEMUSaveHeader { char magic[sizeof(QEMU_SAVE_MAGIC)-1]; -- 2.41.0
On Fri, Sep 01, 2023 at 10:32:13 +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 17 ----------------- src/qemu/qemu_saveimage.h | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 2 +- src/qemu/qemu_saveimage.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index eca47171c2..44ab263144 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -207,7 +207,7 @@ virQEMUSaveDataFinish(virQEMUSaveData *data, } -static virCommand * +virCommand * qemuSaveImageGetCompressionCommand(virQEMUSaveFormat compression) { virCommand *ret = NULL; diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 6892e6764f..510576baa2 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -105,6 +105,9 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat, bool use_raw_on_fail) ATTRIBUTE_NONNULL(2); +virCommand * +qemuSaveImageGetCompressionCommand(virQEMUSaveFormat compression); + int qemuSaveImageCreate(virQEMUDriver *driver, virDomainObj *vm, -- 2.41.0
On Fri, Sep 01, 2023 at 10:32:14 +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 2 +- src/qemu/qemu_saveimage.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)
Also squashable into the previous commit since you need both for the same reason. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Original code assumed that the memory state file is only migration stream but it has additional metadata stored by libvirt. To correctly load the memory state file we need to reuse code that is used when restoring domain from saved image. This duplicates some necessary parts of qemuSaveImageStartVM() because the external snapshot memory state is done by qemuSaveImageCreate(). Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 115 +++++++++++++++++++++++++++++++++------ 1 file changed, 97 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index ff85d56572..538da6570a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2004,6 +2004,22 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +typedef struct _qemuSnapshotRevertMemoryData { + int fd; + char *path; + virQEMUSaveData *data; +} qemuSnapshotRevertMemoryData; + + +static void +qemuSnapshotClearRevertMemoryData(qemuSnapshotRevertMemoryData *memdata) +{ + VIR_FORCE_CLOSE(memdata->fd); + virQEMUSaveDataFree(memdata->data); +} +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(qemuSnapshotRevertMemoryData, qemuSnapshotClearRevertMemoryData); + + /** * qemuSnapshotRevertExternalPrepare: * @vm: domain object @@ -2011,15 +2027,13 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, * @snap: snapshot object we are reverting to * @config: live domain definition * @inactiveConfig: offline domain definition - * memsnapFD: pointer to store memory state file FD or NULL - * memsnapPath: pointer to store memory state file path or NULL + * @memdata: struct with data to load memory state * * Prepare new temporary snapshot definition @tmpsnapdef that will * be used while creating new overlay files after reverting to snapshot * @snap. In case we are reverting to snapshot with memory state it will - * open it and pass FD via @memsnapFD and path to the file via - * @memsnapPath, caller is responsible for freeing both @memsnapFD and - * memsnapPath. + * open it and store necessary data in @memdata. Caller is responsible + * to clear the data by using qemuSnapshotClearRevertMemoryData(). * * Returns 0 in success, -1 on error. */ @@ -2029,8 +2043,7 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainMomentObj *snap, virDomainDef *config, virDomainDef *inactiveConfig, - int *memsnapFD, - char **memsnapPath) + qemuSnapshotRevertMemoryData *memdata) { size_t i; bool active = virDomainObjIsActive(vm); @@ -2065,12 +2078,20 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, return -1; } - if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + if (memdata && snapdef->memorysnapshotfile) { virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virDomainDef) savedef = NULL; - *memsnapPath = snapdef->memorysnapshotfile; - *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + memdata->path = snapdef->memorysnapshotfile; + memdata->fd = qemuSaveImageOpen(driver, NULL, memdata->path, + &savedef, &memdata->data, + false, NULL, false, false); + + if (memdata->fd < 0) + return -1; + + if (!virDomainDefCheckABIStability(savedef, domdef, driver->xmlopt)) + return -1; } return 0; @@ -2254,13 +2275,16 @@ qemuSnapshotRevertActive(virDomainObj *vm, virObjectEvent *event = NULL; virObjectEvent *event2 = NULL; virDomainMomentObj *loadSnap = NULL; - VIR_AUTOCLOSE memsnapFD = -1; - char *memsnapPath = NULL; int detail; bool defined = false; qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie; int rc; g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_auto(qemuSnapshotRevertMemoryData) memdata = { -1, NULL, NULL }; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *errbuf = NULL; + VIR_AUTOCLOSE intermediatefd = -1; + int memrc = 0; start_flags |= VIR_QEMU_PROCESS_START_PAUSED; @@ -2284,7 +2308,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, *config, *inactiveConfig, - &memsnapFD, &memsnapPath) < 0) { + &memdata) < 0) { return -1; } } else { @@ -2298,6 +2322,30 @@ qemuSnapshotRevertActive(virDomainObj *vm, virDomainObjAssignDef(vm, config, true, NULL); + if (virDomainSnapshotIsExternal(snap) && memdata.data) { + virQEMUSaveHeader *header = &memdata.data->header; + + if (header && (header->version == 2) && + (header->compressed != QEMU_SAVE_FORMAT_RAW)) { + if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed))) + return -1; + + intermediatefd = memdata.fd; + memdata.fd = -1; + + virCommandSetInputFD(cmd, intermediatefd); + virCommandSetOutputFD(cmd, &memdata.fd); + virCommandSetErrorBuffer(cmd, &errbuf); + virCommandDoAsyncIO(cmd); + + if (virCommandRunAsync(cmd, NULL) < 0) { + memdata.fd = intermediatefd; + intermediatefd = -1; + return -1; + } + } + } + /* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. */ @@ -2307,17 +2355,48 @@ qemuSnapshotRevertActive(virDomainObj *vm, rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_SNAPSHOT, NULL, memsnapFD, - memsnapPath, loadSnap, + VIR_ASYNC_JOB_SNAPSHOT, "stdio", + memdata.fd, memdata.path, loadSnap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); + + if (virDomainSnapshotIsExternal(snap) && memdata.data) { + if (intermediatefd != -1) { + virErrorPtr orig_err = NULL; + + if (rc < 0) { + /* if there was an error setting up qemu, the intermediate + * process will wait forever to write to stdout, so we + * must manually kill it and ignore any error related to + * the process + */ + virErrorPreserveLast(&orig_err); + VIR_FORCE_CLOSE(intermediatefd); + VIR_FORCE_CLOSE(memdata.fd); + } + + memrc = virCommandWait(cmd, NULL); + VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf)); + virErrorRestore(&orig_err); + } + if (VIR_CLOSE(memdata.fd) < 0) { + virReportSystemError(errno, _("cannot close file: %1$s"), memdata.path); + memrc = -1; + } + + /* qemuProcessStart doesn't unset the qemu error reporting infrastructure + * in case of migration (which is used in this case) so we need to reset it + * so that the handle to virtlogd is not held open unnecessarily */ + qemuMonitorSetDomainLog(qemuDomainGetMonitor(vm), NULL, NULL, NULL); + } + virDomainAuditStart(vm, "from-snapshot", rc >= 0); detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, detail); virObjectEventStateQueue(driver->domainEventState, event); - if (rc < 0) + if (rc < 0 || memrc < 0) return -1; @@ -2428,7 +2507,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, NULL, *inactiveConfig, - NULL, NULL) < 0) { + NULL) < 0) { return -1; } -- 2.41.0
On Fri, Sep 01, 2023 at 10:32:15 +0200, Pavel Hrdina wrote:
Original code assumed that the memory state file is only migration stream but it has additional metadata stored by libvirt. To correctly load the memory state file we need to reuse code that is used when restoring domain from saved image.
This duplicates some necessary parts of qemuSaveImageStartVM() because the external snapshot memory state is done by qemuSaveImageCreate().
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 115 +++++++++++++++++++++++++++++++++------ 1 file changed, 97 insertions(+), 18 deletions(-)
Hmmm, in the end you were right. This version is more verbose and in fact doesn't end up having two code paths for external and internal but both call qemuProcessStart which still does everything together. I'll re-visit v1. Since my main concern is not aleviated by this one and I think v1 can be made tolerable by improving some function documentation. Regardless of the above: Reviewed-by: Peter Krempa <pkrempa@redhat.com> as I went through this pathc already.
On Fri, Sep 01, 2023 at 10:32:15 +0200, Pavel Hrdina wrote:
Original code assumed that the memory state file is only migration stream but it has additional metadata stored by libvirt. To correctly load the memory state file we need to reuse code that is used when restoring domain from saved image.
This duplicates some necessary parts of qemuSaveImageStartVM() because the external snapshot memory state is done by qemuSaveImageCreate().
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 115 +++++++++++++++++++++++++++++++++------ 1 file changed, 97 insertions(+), 18 deletions(-)
[...]
/* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. */ @@ -2307,17 +2355,48 @@ qemuSnapshotRevertActive(virDomainObj *vm,
rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL,
As noted in review of v1, we ought to use the cookie from the save image or at least validate that this one is correct.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- NEWS.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index e40c8ac259..a3be76d6cc 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -28,6 +28,14 @@ v9.7.0 (unreleased) 2) pre-binding the variant driver using the ``--driver`` option of ``virsh nodedev-detach``. + * QEMU: implement reverting external snapshots + + Reverting external snapshots is now possible using the existing API + ``virDomainSnapshotRevert()``. Management application can check host + capabilities for ``<externalSnapshot/>`` element within the list of + guest features to see if the current libvirt supports both deleting + and reverting external snapshots. + * **Improvements** * **Bug fixes** -- 2.41.0
On Fri, Sep 01, 2023 at 10:32:16 +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- NEWS.rst | 8 ++++++++ 1 file changed, 8 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reverting external snapshot for running VM doesn't work correctly so we should not report this capability until it is fixed. This reverts commit de71573bfec7f3acd22ec74794318de121716e21. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatcaps.rst | 7 ------- src/conf/capabilities.c | 1 - src/conf/capabilities.h | 1 - src/conf/schemas/capability.rng | 5 ----- src/qemu/qemu_capabilities.c | 1 - tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml | 1 - tests/qemucaps2xmloutdata/caps.aarch64.xml | 1 - tests/qemucaps2xmloutdata/caps.ppc.xml | 1 - tests/qemucaps2xmloutdata/caps.ppc64.xml | 1 - tests/qemucaps2xmloutdata/caps.riscv64.xml | 1 - tests/qemucaps2xmloutdata/caps.s390x.xml | 1 - tests/qemucaps2xmloutdata/caps.sparc.xml | 1 - tests/qemucaps2xmloutdata/caps.x86_64+hvf.xml | 1 - tests/qemucaps2xmloutdata/caps.x86_64.xml | 1 - 14 files changed, 24 deletions(-) diff --git a/docs/formatcaps.rst b/docs/formatcaps.rst index 95502c511f..bb8bc663d2 100644 --- a/docs/formatcaps.rst +++ b/docs/formatcaps.rst @@ -134,12 +134,6 @@ The ``<guest/>`` element will typically wrap up the following elements: creating external disk snapshots is supported. If absent, creating external snapshots may still be supported, but it requires attempting the API and checking for an error to find out for sure. :since:`Since 1.2.3` - ``externalSnapshot`` - If this element is present, the hypervisor supports deleting and - reverting external snapshots including memory state. Support for creation - of external snapshots is reported via the ``disksnapshot`` feature flag. - Management applications can now switch from internal snapshots to external - snapshots. :since:`Since 9.7.0` Examples ~~~~~~~~ @@ -324,7 +318,6 @@ capabilities enabled in the chip and BIOS you will see: <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 34f04cb7d3..56768ce6e0 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -508,7 +508,6 @@ static const struct virCapsGuestFeatureInfo virCapsGuestFeatureInfos[VIR_CAPS_GU [VIR_CAPS_GUEST_FEATURE_TYPE_DEVICEBOOT] = { "deviceboot", false }, [VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT] = { "disksnapshot", true }, [VIR_CAPS_GUEST_FEATURE_TYPE_HAP] = { "hap", true }, - [VIR_CAPS_GUEST_FEATURE_TYPE_EXTERNAL_SNAPSHOT] = { "externalSnapshot", false }, }; diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 9eaf6e2807..c78e3e52fa 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -40,7 +40,6 @@ typedef enum { VIR_CAPS_GUEST_FEATURE_TYPE_DEVICEBOOT, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, VIR_CAPS_GUEST_FEATURE_TYPE_HAP, - VIR_CAPS_GUEST_FEATURE_TYPE_EXTERNAL_SNAPSHOT, VIR_CAPS_GUEST_FEATURE_TYPE_LAST } virCapsGuestFeatureType; diff --git a/src/conf/schemas/capability.rng b/src/conf/schemas/capability.rng index b1968df258..83b414961a 100644 --- a/src/conf/schemas/capability.rng +++ b/src/conf/schemas/capability.rng @@ -493,11 +493,6 @@ <empty/> </element> </optional> - <optional> - <element name="externalSnapshot"> - <empty/> - </element> - </optional> </interleave> </element> </define> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 05cc11218a..40eacf0f4d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1121,7 +1121,6 @@ virQEMUCapsInitGuestFromBinary(virCaps *caps, virCapabilitiesAddGuestFeature(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DEVICEBOOT); virCapabilitiesAddGuestFeatureWithToggle(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, true, false); - virCapabilitiesAddGuestFeature(guest, VIR_CAPS_GUEST_FEATURE_TYPE_EXTERNAL_SNAPSHOT); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG)) { virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, diff --git a/tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml b/tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml index b9a5b5a1d6..b53a886b90 100644 --- a/tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml +++ b/tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml @@ -21,7 +21,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/tests/qemucaps2xmloutdata/caps.aarch64.xml b/tests/qemucaps2xmloutdata/caps.aarch64.xml index 61512ed740..5dca6d3102 100644 --- a/tests/qemucaps2xmloutdata/caps.aarch64.xml +++ b/tests/qemucaps2xmloutdata/caps.aarch64.xml @@ -21,7 +21,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/tests/qemucaps2xmloutdata/caps.ppc.xml b/tests/qemucaps2xmloutdata/caps.ppc.xml index 86d6b85ae0..e7ba391795 100644 --- a/tests/qemucaps2xmloutdata/caps.ppc.xml +++ b/tests/qemucaps2xmloutdata/caps.ppc.xml @@ -19,7 +19,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/tests/qemucaps2xmloutdata/caps.ppc64.xml b/tests/qemucaps2xmloutdata/caps.ppc64.xml index 90859f9594..85623f3980 100644 --- a/tests/qemucaps2xmloutdata/caps.ppc64.xml +++ b/tests/qemucaps2xmloutdata/caps.ppc64.xml @@ -20,7 +20,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/tests/qemucaps2xmloutdata/caps.riscv64.xml b/tests/qemucaps2xmloutdata/caps.riscv64.xml index c6fa950211..09b7eb7f2f 100644 --- a/tests/qemucaps2xmloutdata/caps.riscv64.xml +++ b/tests/qemucaps2xmloutdata/caps.riscv64.xml @@ -19,7 +19,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/tests/qemucaps2xmloutdata/caps.s390x.xml b/tests/qemucaps2xmloutdata/caps.s390x.xml index 6379ab73e6..bb82a15040 100644 --- a/tests/qemucaps2xmloutdata/caps.s390x.xml +++ b/tests/qemucaps2xmloutdata/caps.s390x.xml @@ -20,7 +20,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/tests/qemucaps2xmloutdata/caps.sparc.xml b/tests/qemucaps2xmloutdata/caps.sparc.xml index b5b158e430..9d977c4102 100644 --- a/tests/qemucaps2xmloutdata/caps.sparc.xml +++ b/tests/qemucaps2xmloutdata/caps.sparc.xml @@ -19,7 +19,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/tests/qemucaps2xmloutdata/caps.x86_64+hvf.xml b/tests/qemucaps2xmloutdata/caps.x86_64+hvf.xml index f5e49ba4db..356819a6c6 100644 --- a/tests/qemucaps2xmloutdata/caps.x86_64+hvf.xml +++ b/tests/qemucaps2xmloutdata/caps.x86_64+hvf.xml @@ -22,7 +22,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> diff --git a/tests/qemucaps2xmloutdata/caps.x86_64.xml b/tests/qemucaps2xmloutdata/caps.x86_64.xml index 8dd1439a80..35359780c4 100644 --- a/tests/qemucaps2xmloutdata/caps.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps.x86_64.xml @@ -22,7 +22,6 @@ <cpuselection/> <deviceboot/> <disksnapshot default='on' toggle='no'/> - <externalSnapshot/> </features> </guest> -- 2.41.0
On Fri, Sep 01, 2023 at 10:32:17 +0200, Pavel Hrdina wrote:
Reverting external snapshot for running VM doesn't work correctly so we should not report this capability until it is fixed.
This reverts commit de71573bfec7f3acd22ec74794318de121716e21.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
If Jirka decides to go with this alternative and cut the release before the fixes: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Fri, Sep 01, 2023 at 11:24:28 +0200, Peter Krempa wrote:
On Fri, Sep 01, 2023 at 10:32:17 +0200, Pavel Hrdina wrote:
Reverting external snapshot for running VM doesn't work correctly so we should not report this capability until it is fixed.
This reverts commit de71573bfec7f3acd22ec74794318de121716e21.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
If Jirka decides to go with this alternative and cut the release before the fixes:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Yeah, I think this is a better approach for this release. The functionality can be properly fixed after the release. Jirka
participants (3)
-
Jiri Denemark -
Pavel Hrdina -
Peter Krempa