Re: [libvirt] [PATCH 1/1] qemu: snapshot: Forbid internal snapshots with OVMF firmware

On 03/22/17 16:28, Peter Krempa wrote: > QEMU does not snapshot the pflash drive when doing a 'savevm' thus > internal snapshots with OVMF would be incomplete. Can you please explain it in a bit more detail: - the above statement is true as long as we use "raw" for the varstore. - it would not be true if we used "qcow2"; however, qcow2 cannot be used, because QEMU then unpredictably dumps the full vmstate into the varstore drive, which is a very bad thing. And *that* is the ultimate reason for naming QEMU in the commit message. > > Forbid such snapshot so that we can avoid problems. > --- > CC: lersek(a)redhat.com > > There might be slight regression potential. While this did not work as expected s/did not work/did work/?

> I did not encounter any error while doing a savevm, thus users might actually > think that this worked before. > > 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 0e065081d..344917451 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 */ Please update the comment similarly to the commit message. Also, since my inner pedant is having a field day today, please replace the "OVMF" string with "edk2", as the same situation is true for aarch64/AAVMF. (Feel free to ignore this remark if you feel "edk2" is a reference too obscure for the libvirt codebase.) > + if (found_internal && > + vm->def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) { The logic seems fine. > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("internal snapshots of a VM with pflash based " > + "firmware are not supported by QEMU")); Please drop "by QEMU". This statement is a simplification of affairs, and we cannot "blame" QEMU without providing more details. QEMU would be perfectly happy with snapshotting a qcow2-formatted pflash drive *if* we were happy with QEMU unpredictably dumping multiple gigabytes of vmstate into the same drive. Putting the full (updated) commit message in the error string would be lame, so let's stay both succinct *and* precise -- let's just not mention QEMU here. > + goto cleanup; > + } > + > /* Alter flags to let later users know what we learned. */ > if (external && !active) > *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; > Thanks! Laszlo