On Wed, Feb 26, 2025 at 13:51:51 +0100, Pavel Hrdina wrote: > 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@xxxxxxxxxx> > > > --- > > > 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. So after some off-list discussion we also establised that the reversion step creates overlays even for readonly disks (which is sub-optimal although was like that ever since the reversion code was implemented ... > > > > + > > > + 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. ... in which case this check is actually equivalent. Please add the following comments for anyone having to touch this: 1) here stating that the reversion code indeed makes overlays on top of everything and to see qemuSnapshotRevertExternalPrepare 2) in qemuSnapshotRevertExternalPrepare linking back here noting that if someone were to change the above logic to also fix the check With that: Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
