Historically the snapshot code attempted to forbid internal snapshots
with UEFI both in active and inactive case. Unfortunately due to the
intricacies of UEFI probing this didn't really work for inactive VMs
which made users rely on the feature.

Now with the changes to store detected UEFI environment also in the
inactive definition this broke the feature for those users.

Since the varstore doesn't really change that much in the lifecycle of a
VM it usually is okay to simply leave it as is.

Restore the functionality for inactive snapshots by disabling the check.

In the future when uefi snapshotting will be added the rest of the
condition will also be removed.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/460
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_snapshot.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 12ddf19c48..91de8b0c31 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -749,11 +749,17 @@ qemuSnapshotPrepare(virDomainObj *vm,
      * - if the variable store is raw, the snapshot fails
      * - allowing a qcow2 image as the varstore would make it eligible to receive
      *   the vmstate dump, which would make it huge
-     * - offline snapshot would not snapshot the varstore at all
      *
-     * Avoid the issues by forbidding internal snapshot with pflash completely.
+     * While offline snapshot would not snapshot the varstore at all, this used
+     * to work as auto-detected UEFI firmware was not present in the offline
+     * definition. Since in most cases the varstore doesn't change it's usually
+     * not an issue. Allow this as there are existing users of this case.
+     *
+     * Avoid the issues by forbidding internal snapshot with pflash if the
+     * VM is active.
      */
-    if (found_internal &&
+    if (active &&
+        found_internal &&
         virDomainDefHasOldStyleUEFI(vm->def)) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("internal snapshots of a VM with pflash based "
-- 
2.39.2

On a Thursday in 2023, Peter Krempa wrote:
>Historically the snapshot code attempted to forbid internal snapshots
>with UEFI both in active and inactive case. Unfortunately due to the
>intricacies of UEFI probing this didn't really work for inactive VMs
>which made users rely on the feature.
>
>Now with the changes to store detected UEFI environment also in the
>inactive definition this broke the feature for those users.
>
>Since the varstore doesn't really change that much in the lifecycle of a
>VM it usually is okay to simply leave it as is.
>
>Restore the functionality for inactive snapshots by disabling the check.
>
>In the future when uefi snapshotting will be added the rest of the
>condition will also be removed.
>
>Resolves: https://gitlab.com/libvirt/libvirt/-/issues/460
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_snapshot.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano