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

Changes in v2:
    - add comments
    - limit the check for external snapshots

 src/qemu/qemu_snapshot.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 891e67e98b..dab841ef5b 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2203,6 +2203,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 &&
@@ -2230,6 +2232,22 @@ qemuSnapshotRevertValidate(virDomainObj *vm,
         }
     }
 
+    /* Reverting to external snapshot creates overlay files for every disk and
+     * it would fail for non-file based disks.
+     * See qemuSnapshotRevertExternalPrepare for more details. */
+    if (virDomainSnapshotIsExternal(snap)) {
+        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 regular file, reverting to snapshot is not supported"),
+                               disk->dst);
+                return -1;
+            }
+        }
+    }
+
     return 0;
 }
 
@@ -2381,6 +2399,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
     if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0)
         return -1;
 
+    /* Force default location to be external in order to create overlay files
+     * for every disk. In qemuSnapshotRevertValidate we make sure that each
+     * disk is regular file otherwise this would fail. */
     if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef,
                                     VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL,
                                     false, true) < 0) {
-- 
2.48.1

On Thu, Mar 06, 2025 at 14:39:41 +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>
> ---
> 
> Changes in v2:
>     - add comments
>     - limit the check for external snapshots
> 
>  src/qemu/qemu_snapshot.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 891e67e98b..dab841ef5b 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2203,6 +2203,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 &&
> @@ -2230,6 +2232,22 @@ qemuSnapshotRevertValidate(virDomainObj *vm,
>          }
>      }
>  
> +    /* Reverting to external snapshot creates overlay files for every disk and
> +     * it would fail for non-file based disks.
> +     * See qemuSnapshotRevertExternalPrepare for more details. */
> +    if (virDomainSnapshotIsExternal(snap)) {
> +        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 regular file, reverting to snapshot is not supported"),

Perhaps "not yet supported"? :)

Or you don't want to promise anything?

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

Reviewed-by: Peter Krempa <pkrempa@redhat.com>