1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH] qemu: snapshot: error out early when reverting snapshot for VM with non-file disk

Pavel Hrdina posted 1 patch 1 week, 6 days ago
There is a newer version of this series
src/qemu/qemu_snapshot.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
[libvirt PATCH] qemu: snapshot: error out early when reverting snapshot for VM with non-file disk
Posted by Pavel Hrdina 1 week, 6 days ago 
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@redhat.com>
---
 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
Re: [libvirt PATCH] qemu: snapshot: error out early when reverting snapshot for VM with non-file disk
Posted by Peter Krempa 1 week, 6 days ago 
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@redhat.com>
> ---
>  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' ...

> +
> +        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.

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


*regular



> +                           disk->dst);
> +            return -1;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> -- 
> 2.48.1
>
Re: [libvirt PATCH] qemu: snapshot: error out early when reverting snapshot for VM with non-file disk
Posted by Pavel Hrdina 1 week, 6 days ago 
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@redhat.com>
> > ---
> >  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.
> 
> > +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > +                           _("source disk for '%1$s' is not a regural file, reverting to snapshot is not supported"),
> 
> 
> *regular
> 
> 
> 
> > +                           disk->dst);
> > +            return -1;
> > +        }
> > +    }
> > +
> >      return 0;
> >  }
> >  
> > -- 
> > 2.48.1
> > 
>
Re: [libvirt PATCH] qemu: snapshot: error out early when reverting snapshot for VM with non-file disk
Posted by Peter Krempa 1 week, 5 days ago 
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@redhat.com>
> > > ---
> > >  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@redhat.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.