Before this patch the code would start the revert process by destroying the VM and preparing to revert where it would fail with following error: error: unsupported configuration: source for disk 'sdb' is not a regular file; refusing to generate external snapshot name and leaving user with offline VM even if it was running. Make the check before we start the revert process to not destroy VMs. Resolves: https://issues.redhat.com/browse/RHEL-30971 Resolves: https://issues.redhat.com/browse/RHEL-79928 Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index f7d6272907..c5f70f5b10 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2205,6 +2205,8 @@ qemuSnapshotRevertValidate(virDomainObj *vm, virDomainSnapshotDef *snapdef, unsigned int flags) { + size_t i; + if (!vm->persistent && snapdef->state != VIR_DOMAIN_SNAPSHOT_RUNNING && snapdef->state != VIR_DOMAIN_SNAPSHOT_PAUSED && @@ -2232,6 +2234,17 @@ qemuSnapshotRevertValidate(virDomainObj *vm, } } + for (i = 0; i < snap->def->dom->ndisks; i++) { + virDomainDiskDef *disk = snap->def->dom->disks[i];

+ + if (disk->src->type != VIR_STORAGE_TYPE_FILE) {

+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("source disk for '%1$s' is not a regural file, reverting to snapshot is not supported"),

+ disk->dst); + return -1; + } + } + return 0; } -- 2.48.1

On Wed, Feb 26, 2025 at 12:29:14PM +0100, Peter Krempa wrote: > On Wed, Feb 26, 2025 at 11:33:52 +0100, Pavel Hrdina wrote: > > Before this patch the code would start the revert process by destroying > > the VM and preparing to revert where it would fail with following error: > > > > error: unsupported configuration: source for disk 'sdb' is not a regular file; refusing to generate external snapshot name > > > > and leaving user with offline VM even if it was running. > > > > Make the check before we start the revert process to not destroy VMs. > > > > Resolves: https://issues.redhat.com/browse/RHEL-30971 > > Resolves: https://issues.redhat.com/browse/RHEL-79928 > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > > --- > > src/qemu/qemu_snapshot.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c > > index f7d6272907..c5f70f5b10 100644 > > --- a/src/qemu/qemu_snapshot.c > > +++ b/src/qemu/qemu_snapshot.c > > @@ -2205,6 +2205,8 @@ qemuSnapshotRevertValidate(virDomainObj *vm, > > virDomainSnapshotDef *snapdef, > > unsigned int flags) > > { > > + size_t i; > > + > > if (!vm->persistent && > > snapdef->state != VIR_DOMAIN_SNAPSHOT_RUNNING && > > snapdef->state != VIR_DOMAIN_SNAPSHOT_PAUSED && > > @@ -2232,6 +2234,17 @@ qemuSnapshotRevertValidate(virDomainObj *vm, > > } > > } > > > > + for (i = 0; i < snap->def->dom->ndisks; i++) { > > + virDomainDiskDef *disk = snap->def->dom->disks[i]; > > This code isn't doing the same logic as > qemuSnapshotRevertExternalPrepare as it doesn't skip disks that weren't > originally selected for snapshot in 'snapdef' ... When reverting to any external snapshot we don't take into consideration which disks were originally excluded and libvirt want's to create overlay for every disk. The reasoning is that skipping these disks could still lead to data corruption, for example consider this situation: snapshot1 disk1 -> external disk2 -> ignored snapshot2 disk1 -> external disk2 -> external When reverting to snapshot1 we cannot skip disk2 so when reverting was implemented we decided that new qcow2 overlay would be always created. The other option would be adding logic that would try to detect if new overlay is required or not. Because of that reason function qemuSnapshotRevertExternalPrepare would skip this disk but it also calls virDomainSnapshotAlignDisks before the skip code where any ignored disk in the original snapshot is included even if VM definition disables snapshots for some disks.

> > + > > + if (disk->src->type != VIR_STORAGE_TYPE_FILE) { > > ... so this'd in such case refuse the revert even if the disk state > wouldn't in fact be reverted.