Re: [PATCH 5/6] qemuSnapshotCreateActiveExternalDisks: Extract actual snapshot creation to 'qemuSnapshotDiskCreate'

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Thursday in 2020, Peter Krempa wrote:
> Extract the code which invokes the monitor and finalizes the snapshot
> into a separate function for easier reuse.
>
> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> ---
> src/qemu/qemu_snapshot.c | 67 ++++++++++++++++++++++++----------------
> 1 file changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index b9b640058c..6fe925763e 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -1182,57 +1182,42 @@ qemuSnapshotDiskUpdateSource(virDomainObjPtr vm,
> }
>
>
> -/* The domain is expected to be locked and active. */
> static int
> -qemuSnapshotCreateActiveExternalDisks(virQEMUDriverPtr driver,
> -                                      virDomainObjPtr vm,
> -                                      virDomainMomentObjPtr snap,
> -                                      virHashTablePtr blockNamedNodeData,
> -                                      unsigned int flags,
> -                                      virQEMUDriverConfigPtr cfg,
> -                                      qemuDomainAsyncJob asyncJob)
> +qemuSnapshotDiskCreate(qemuSnapshotDiskContextPtr snapctxt,
> +                       virQEMUDriverConfigPtr cfg)
> {
> -    qemuDomainObjPrivatePtr priv = vm->privateData;
> -    int rc;
> +    qemuDomainObjPrivatePtr priv = snapctxt->vm->privateData;
> +    virQEMUDriverPtr driver = priv->driver;
>     size_t i;
> -    bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
> -    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
> -
> -    if (virDomainObjCheckActive(vm) < 0)
> -        return -1;
> +    int rc;
>
> -    /* prepare a list of objects to use in the vm definition so that we don't
> -     * have to roll back later */
> -    if (!(snapctxt = qemuSnapshotDiskPrepare(vm, snap, cfg, reuse,
> -                                             blockNamedNodeData, asyncJob)))
> -        return -1;

This leaves a double space here.

Jano

>     /* check whether there's anything to do */
>     if (snapctxt->ndd == 0)
>         return 0;
>
> -    if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> +    if (qemuDomainObjEnterMonitorAsync(driver, snapctxt->vm, snapctxt->asyncJob) < 0)
>         return -1;
>
>     rc = qemuMonitorTransaction(priv->mon, &snapctxt->actions);
>
> -    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +    if (qemuDomainObjExitMonitor(driver, snapctxt->vm) < 0)
>         rc = -1;
>
>     for (i = 0; i < snapctxt->ndd; i++) {
>         qemuSnapshotDiskDataPtr dd = snapctxt->dd + i;
>
> -        virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0);
> +        virDomainAuditDisk(snapctxt->vm, dd->disk->src, dd->src, "snapshot", rc >= 0);
>
>         if (rc == 0)
> -            qemuSnapshotDiskUpdateSource(vm, dd);
> +            qemuSnapshotDiskUpdateSource(snapctxt->vm, dd);
>     }
>
>     if (rc < 0)
>         return -1;
>
> -    if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0 ||
> -        (vm->newDef && virDomainDefSave(vm->newDef, driver->xmlopt,
> +    if (virDomainObjSave(snapctxt->vm, driver->xmlopt, cfg->stateDir) < 0 ||
> +        (snapctxt->vm->newDef && virDomainDefSave(snapctxt->vm->newDef, driver->xmlopt,
>                                         cfg->configDir) < 0))
>         return -1;
>
> @@ -1240,6 +1225,34 @@ qemuSnapshotCreateActiveExternalDisks(virQEMUDriverPtr driver,
> }
>
>
> +/* The domain is expected to be locked and active. */
> +static int
> +qemuSnapshotCreateActiveExternalDisks(virDomainObjPtr vm,
> +                                      virDomainMomentObjPtr snap,
> +                                      virHashTablePtr blockNamedNodeData,
> +                                      unsigned int flags,
> +                                      virQEMUDriverConfigPtr cfg,
> +                                      qemuDomainAsyncJob asyncJob)
> +{
> +    bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
> +    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
> +
> +    if (virDomainObjCheckActive(vm) < 0)
> +        return -1;
> +
> +    /* prepare a list of objects to use in the vm definition so that we don't
> +     * have to roll back later */
> +    if (!(snapctxt = qemuSnapshotDiskPrepare(vm, snap, cfg, reuse,
> +                                             blockNamedNodeData, asyncJob)))
> +        return -1;
> +
> +    if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
> static int
> qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>                                  virDomainObjPtr vm,
> @@ -1366,7 +1379,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>
>     /* the domain is now paused if a memory snapshot was requested */
>
> -    if ((ret = qemuSnapshotCreateActiveExternalDisks(driver, vm, snap,
> +    if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap,
>                                                      blockNamedNodeData, flags, cfg,
>                                                      QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
>         goto cleanup;
> --
> 2.26.2

Attachment: signature.asc
Description: PGP signature