[PATCH v2 2/7] qemu: Introduce functions to handle transient disk

[Peter Krempa]

On Fri, Aug 28, 2020 at 10:08:31 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> 
> Here is the implementation of transient option for qcow2 and raw format
> disk. This gets available <transient/> directive in domain xml file
> like as:
> 
>     <disk type='file' device='disk'>
>       <driver name='qemu' type='qcow2'/>
>       <source file='/var/lib/libvirt/images/guest.qcow2'/>
>       <target dev='vda' bus='virtio'/>
>       <transient/>
>     </disk>
> 
> When the qemu command line options are built, a new qcow2 image is
> created with backing qcow2 by using blockdev-snapshot command.
> The backing image is the qcow2 file which is set as <source>.
> The filename of the new qcow2 image is original-source-file.TRANSIENT.
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> ---
>  src/qemu/qemu_snapshot.c  | 134 ++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_snapshot.h  |   8 +++
>  src/util/virstoragefile.h |   2 +
>  3 files changed, 144 insertions(+)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index d310e6ff02..5c61d19f26 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2265,3 +2265,137 @@ qemuSnapshotDelete(virDomainObjPtr vm,
>   cleanup:
>      return ret;
>  }
> +
> +static int
> +qemuTransientDiskPrepareOne(virQEMUDriverPtr driver,

This should be named with a qemuSnapshot... prefix:

qemuSnapshotDiskTransientPrepareOne for example.

> +                            virDomainObjPtr vm,
> +                            qemuSnapshotDiskDataPtr data,
> +                            virHashTablePtr blockNamedNodeData,
> +                            int asyncJob,
> +                            virJSONValuePtr actions)
> +{
> +    int rc = -1;
> +    virStorageSourcePtr dest;

You can use g_autoptr here.

> +    virStorageSourcePtr src = data->disk->src;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
> +
> +    if (!strlen(src->path))

This will not work as expected. You must make sure that the source disk
is VIR_STORAGE_TYPE_FILE. presence of src->path does not guarantee it.

Also you can use virStorageSourceIsEmpty() here if you want to skip
empty cdroms, but transient cdrom/readonly disks should be rejected in
the config validator beforehand.

> +        return rc;

Please always return constants right away. (rc is a constant at this
point).

> +
> +    if (!(dest = virStorageSourceNew()))
> +        return rc;
> +
> +    dest->path = g_strdup_printf("%s.TRANSIENT", src->path);
> +
> +    if (virFileExists(dest->path)) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("Transient disk '%s' for '%s' exists"),
> +                       dest->path, src->path);
> +        goto cleanup;
> +    }
> +
> +    dest->type = VIR_STORAGE_TYPE_FILE;
> +    dest->format = VIR_STORAGE_FILE_QCOW2;
> +    data->src = dest;
> +
> +    if (qemuSnapshotDiskPrepareOne(driver, vm, cfg, data->disk,
> +                                         NULL, data, blockNamedNodeData,

Create a virDomainSnapshotDiskDefPtr with the correct data from above
and pass it in here, rather than working around the addition to the data
structure.

> +                                         false, true, asyncJob, actions) < 0)
> +        goto cleanup;
> +
> +    rc = 0;
> + cleanup:
> +    if (rc < 0)
> +        g_free(dest->path);
> +
> +    return rc;
> +}
> +
> +static int
> +qemuWaitTransaction(virQEMUDriverPtr driver,
> +                    virDomainObjPtr vm,
> +                    int asyncJob,
> +                    virJSONValuePtr *actions)

This function isn't IMO necessary.

> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +    if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> +        return -1;
> +
> +    if (qemuMonitorTransaction(priv->mon, actions) < 0)
> +        return -1;
> +
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        return -1;
> +
> +    rturn 0;
> +}
> +
> +int
> +qemuTransientCreatetDisk(virQEMUDriverPtr driver,
> +                         virDomainObjPtr vm,
> +                         int asyncJob)
> +{
> +    size_t i;
> +    int rc = -1;
> +    size_t ndata = 0;
> +    qemuSnapshotDiskDataPtr data = NULL;
> +    g_autoptr(virJSONValue) actions = NULL;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    g_autoptr(virHashTable) blockNamedNodeData = NULL;
> +    bool blockdev =  virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);

Imo it will be better to factor out the common parts from this and
qemuSnapshotCreateDiskActive and then just use the disjunct logic in
this function.

> +
> +    if (!blockdev)
> +        return rc;

This breaks startup of VMs with pre-blockdev qemu as it returns -1 here
directly when blockdev is not supported.

> +    if (VIR_ALLOC_N(data, vm->def->ndisks) < 0)
> +        return rc;
> +
> +    if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob)))
> +        goto cleanup;
> +
> +    if (!(actions = virJSONValueNewArray()))
> +        goto cleanup;
> +
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        virDomainDiskDefPtr disk = vm->def->disks[i];
> +
> +        if (disk->src->readonly)
> +            continue;

This transient+readonly should be a hard error and validated in the
config validator rather than here.

> +
> +        if (disk->transient) {
> +            data[ndata].disk = disk;
> +            if (qemuTransientDiskPrepareOne(driver, vm, &data[ndata], blockNamedNodeData,
> +                                                 asyncJob, actions) < 0)

misaligned code.

> +                goto cleanup;
> +            ndata++;
> +        }
> +    }
> +
> +    if (qemuWaitTransaction(driver, vm, asyncJob, &actions) < 0)
> +        goto cleanup;
> +
> +    for (i = 0; i < ndata; i++) {
> +        qemuSnapshotDiskDataPtr dd = &data[i];
> +
> +        qemuSnapshotDiskUpdateSource(driver, vm, dd, blockdev);
> +        dd->disk->src->transientEstablished = true;
> +    }

These both can be factored out and shared with the snapshot code.

> +
> +    VIR_FREE(data);
> +    rc = 0;
> + cleanup:
> +    qemuSnapshotDiskCleanup(data, vm->def->ndisks, driver, vm, asyncJob);
> +
> +    return rc;
> +}
> +
> +void
> +qemuTransientRemoveDisk(virDomainDiskDefPtr disk)

This doesn't belong into this helper file. Removal of the snapshot
images is not related to snapshots.

> +{
> +    if (disk->src->transientEstablished) {
> +        VIR_DEBUG("unlink transient disk: %s", disk->src->path);
> +        unlink(disk->src->path);
> +    }
> +}
> diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h
> index 8b3ebe87b1..aecb1762d2 100644
> --- a/src/qemu/qemu_snapshot.h
> +++ b/src/qemu/qemu_snapshot.h
> @@ -53,3 +53,11 @@ int
>  qemuSnapshotDelete(virDomainObjPtr vm,
>                     virDomainSnapshotPtr snapshot,
>                     unsigned int flags);
> +
> +int
> +qemuTransientCreatetDisk(virQEMUDriverPtr driver,
> +                         virDomainObjPtr vm,
> +                         int asyncJob);
> +
> +void
> +qemuTransientRemoveDisk(virDomainDiskDefPtr disk);
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index f73b3ee005..fd42d017f3 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -383,6 +383,8 @@ struct _virStorageSource {
>      /* these must not be used apart from formatting the output JSON in the qemu driver */
>      char *ssh_user;
>      bool ssh_host_key_check_disabled;
> +
> +    bool transientEstablished;

The deep copy function is not updated with this new member. Also you
should document this variable.

>  };
>  
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref);
> -- 
> 2.27.0
>