[libvirt] [PATCH v2] qemu: snapshot: Forbid internal snapshots with pflash firmware

[Laszlo Ersek]

On 03/23/17 10:54, Peter Krempa wrote:
> 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.
> +     */
> 

Yeah I saw the OVMF mention in the v2 comment, despite "dropped mentions
of OVMF", but I figured I wouldn't obsess about it. :)

Anyway, if you post a v3 with the comment updated, you can keep my R-b
with it -- the new comment looks great.

Thanks!
Laszlo