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