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

On Thu, Mar 23, 2017 at 10:48:01 +0100, Laszlo Ersek wrote: > 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; >> I apparently forgot to commit this despite mentioning it in the commit message: @@ -13873,8 +13873,14 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, goto cleanup; } - /* Internal snapshots don't work with VMs with OVMF loader since qemu does - * not snapshot the variable store */ + /* internal snapshots + pflash based loader have the following problems: + * - if the variable store is raw, the snapshot is incomplete + * - alowing 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 this completely. + */