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@xxxxxxxxxx
>>
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
