[libvirt] [PATCH 1/1] qemu: snapshot: Forbid internal snapshots with OVMF firmware
QEMU does not snapshot the pflash drive when doing a 'savevm' thus internal snapshots with OVMF would be incomplete. Forbid such snapshot so that we can avoid problems. --- CC: lersek@redhat.com There might be slight regression potential. While this did not work as expected 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 */ + if (found_internal && + vm->def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("internal snapshots of a VM with pflash based " + "firmware are not supported by QEMU")); + goto cleanup; + } + /* Alter flags to let later users know what we learned. */ if (external && !active) *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; -- 2.12.1
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@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
On 03/22/17 16:38, Laszlo Ersek wrote:
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@redhat.com
There might be slight regression potential. While this did not work as expected
s/did not work/did work/?
Ahhh okay, I understand. With "this", you meant "savevm". I misunderstood "this" as "this patch". OK. Laszlo
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
participants (2)
-
Laszlo Ersek -
Peter Krempa