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

[Laszlo Ersek]

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 at 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
>