[PATCH v2 3/6] qemu: Add internal support for active disk internal snapshots

[Peter Krempa]

On Wed, Feb 15, 2023 at 05:28:19 -0600, Or Ozeri wrote:
> libvirt supports taking external disk snapshots on a running VM,
> using qemu's "blockdev-snapshot" command.
> qemu also supports "blockdev-snapshot-internal-sync" to do the
> same for internal snapshots.
> This commit wraps this (old) qemu capability to allow future libvirt
> users to take internal disk snapshots on a running VM.
> This will only work for disk types which support internal snapshots,
> and thus we require the disk type to be part of a white list of known
> types. For this commit, the list of supported formats is empty.
> An upcoming commit will allow RBD disks to use this new capability.
> 
> Signed-off-by: Or Ozeri <oro at il.ibm.com>
> ---

[...]

There's too much going on in this commit:
 - adds new monitor API and its tests
 - renames function for preparing snapshot data
 - adds the code to do internal snapshot into it
 - modifies checks to report error
 - modifies the external memory snapshot code to work with the new
   snapshot

Split out at least the rename and monitor addition into two separate
patches.

Similarly the bit which shuffles around and prepares the snapshot data
can be done as a separate commit.

> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index c1855b3028..e82352ba7d 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -736,7 +736,7 @@ qemuSnapshotPrepare(virDomainObj *vm,

[...]

> @@ -1229,8 +1247,8 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm,
>  
>      /* prepare a list of objects to use in the vm definition so that we don't
>       * have to roll back later */
> -    if (!(snapctxt = qemuSnapshotDiskPrepareActiveExternal(vm, snap, reuse,
> -                                                           blockNamedNodeData, asyncJob)))
> +    if (!(snapctxt = qemuSnapshotDiskPrepareActive(vm, snap, reuse,
> +                                                   blockNamedNodeData, asyncJob)))
>          return -1;
>  
>      if (qemuSnapshotDiskCreate(snapctxt) < 0)

[...]

> @@ -1370,9 +1388,9 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
>  
>      /* the domain is now paused if a memory snapshot was requested */
>  
> -    if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap,
> -                                                     blockNamedNodeData, flags,
> -                                                     VIR_ASYNC_JOB_SNAPSHOT)) < 0)
> +    if ((ret = qemuSnapshotCreateActiveDisks(vm, snap,
> +                                             blockNamedNodeData, flags,
> +                                             VIR_ASYNC_JOB_SNAPSHOT)) < 0)

This modification is done to a function named
`qemuSnapshotCreateActiveExternal` but clearly enabled creation of
internal snapshots. You'll need to address that one too.

Since your patches add intermingling (at least) partial of snapshots the
code will need to be split up separately.

1) check that the old-style full internal snapshot is requested and deal
with that as a special case
2) separate qemuSnapshotCreateActiveExternal into:
    - 2.1) - the bit that creates the external memory snapshot
    - 2.2) - the bit that creates the external disk snapshot
    - 2.3) - modify the bit that creates external disk snapshot to also
             do the internal disk snapshot for rbd
    - 2.4) - call them in proper sequence. I suspect you also want to do
             a external memory snapshot including the internal RBD
             snapshot as s transaction.

Good bit is that the oldschool internal snapshot is a completely
separate case and can't be intermingled here.

But the existing code must be modified to honour the change of semantics
you are introducing.