[libvirt] [PATCH v2] qemu: snapshot: Forbid internal snapshots with pflash firmware

[Laszlo Ersek]

On 03/23/17 10:29, Peter Krempa wrote:
> If the variable store (<nvram>) file is raw qemu can't do a snapshot of
> it and thus the snapshot would be incomplete. QEMU does no reject such
> snapshot.
> 
> Additionally allowing to use a qcow2 variable store backing file would
> solve this issue but then it would become eligible to become target of
> the memory dump.
> 
> Offline internal snapshot would be incomplete too with either storage
> format since libvirt does not handle the pflash file in this case.
> 
> Forbid such snapshot so that we can avoid problems.
> ---
> 
> Notes:
>     v2:
>     - changed error code to OPERATION_UNSUPPORTED (from CONFIG_UNSUPPORTED)
>     - dropped mention of QEMU from the error message
>     - dropped mentions of OVMF or the firmware itself altoghether, the culprit is
>       the pflash device regardless of the software it contains
>     - mentioned all the stuff in the commit message and comment
>     
>     We also will need to introduce a way to snapshot the pflash for external
>     snapshots which is currently impossible as well, but fortunately does not
>     have inherent drawbacks as internal snapshots.
> 
>  src/qemu/qemu_driver.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 676295208..202d190b3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13873,6 +13873,16 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
>          goto cleanup;
>      }
> 
> +    /* Internal snapshots don't work with VMs with OVMF loader since qemu does
> +     * not snapshot the variable store */
> +    if (found_internal &&
> +        vm->def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("internal snapshots of a VM with pflash based "
> +                         "firmware are not supported"));
> +        goto cleanup;
> +    }
> +
>      /* Alter flags to let later users know what we learned.  */
>      if (external && !active)
>          *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
> 

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Thanks!
Laszlo