On Fri, Apr 10, 2015 at 14:59:03 +0200, Ján Tomko wrote:
> The blockdev-snapshot-sync command only takes a format
> but does not allow specifying the compat level or other features
> that should be used.
>
> Pre-create the qcow2 file ourselves and tell qemu to reuse it.
>
> Note: the default compat level for qemu (and thus external snapshot
> creation) is now 1.10 but libvirt's storage driver still uses 0.10.
> After this patch, 0.10 will be the default for both.
> ---
> src/qemu/qemu_driver.c | 11 ++++++++
> src/storage/storage_backend.c | 66 +++++++++++++++++++++++++++++++++++++++++++
> src/storage/storage_backend.h | 5 ++++
> src/storage/storage_driver.c | 27 ++++++++++++++++++
> src/storage/storage_driver.h | 4 +++
> 5 files changed, 113 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 921417c..4f14546 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14073,6 +14073,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
> char *device = NULL;
> char *source = NULL;
> const char *formatStr = NULL;
> + const char *qemuImgPath = NULL;
> int ret = -1, rc;
> bool need_unlink = false;
>
> @@ -14082,6 +14083,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
> return -1;
> }
>
> + if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
> + goto cleanup;
> +
> if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0)
> goto cleanup;
>
> @@ -14114,6 +14118,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
> goto cleanup;
> }
> need_unlink = true;
> + if (newDiskSrc->format == VIR_STORAGE_FILE_QCOW2) {
> + rc = virStorageFileCreateWithFormat(newDiskSrc,
> + source,
> + disk->src,
> + qemuImgPath);
> + reuse = true;
> + }
Since this step is way more prone to fail compared to the existing
pre-creation step (that is used just to allow labeling the file before
passing it to qemu) I'd rather see this happen prior to actually
starting to take the snapshot (at this point, the memory was already
snapshotted and the VM is possibly paused, so if this takes a long time
or fails we would waste the memory snapshot).
> }
>
> /* set correct security, cgroup and locking options on the new image */
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 4ecea88..9ffbc6e 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1079,6 +1079,72 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
> return cmd;
> }
>
> +
> +/* Create a qemu-img virCommand from the supplied binary path,
> + * StorageSource and path (translated for network drives
> + * supported by qemu-img)
> + */
> +static virCommandPtr
> +virStorageBackendCreateQemuImgCmdFromSource(virStorageSourcePtr src,
> + const char *path,
> + virStorageSourcePtr backingSrc,
> + const char *create_tool)
> +{
> + virCommandPtr cmd = NULL;
> + struct _virStorageBackendQemuImgInfo info = {
> + .format = src->format,
> + .path = path,
> + .encryption = NULL,
> + .preallocate = false,
> + .compat = src->compat,
> + .features = src->features,
> + .nocow = src->nocow,
> + };
> + char *tmpstr = NULL;
> +
> + info.backingFormat = backingSrc->format;
> + info.backingPath = backingSrc->path;
This definitely is not enough to pass the backing source path. Once you
have a network path you need a lot of the fields virStorageSource
structure to format the path since it needs to format the
qemu-compatible backing string.
A better way will be to format the string from backingSrc right at this
point to the qemu source string and use that.
Additionally, you'll also need
> +
> + if (!(tmpstr = virBitmapFormat(info.features)))
> + return NULL;
> +
> + VIR_DEBUG("creating file via qemu_img: format %s path %s compat %s features %s",
> + virStorageFileFormatTypeToString(info.format),
> + info.path, info.compat, tmpstr);
> + VIR_DEBUG("... backing format %s backing path %s",
> + virStorageFileFormatTypeToString(info.backingFormat),
> + info.backingPath);
> +
> + cmd = virStorageBackendCreateQemuImgCmd(create_tool,
> + QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT,
> + info);
> + return cmd;
> +}
> +
> +
> +int
> +virStorageBackendCreateQemuImgFileFromSource(virStorageSourcePtr src,
> + const char *path,
Since you already are passing @src, there should be no need to use
@path.
Additionally since @path contains authentication data for some protocols
we should not pass it to commands that would show in the process list
with the arguments. (We already do that when starting the VM. I have it on
my todo list)
> + virStorageSourcePtr backingSrc,
> + const char *create_tool)
> +{
> + int ret = -1;
> + virCommandPtr cmd = NULL;
> +
> + cmd = virStorageBackendCreateQemuImgCmdFromSource(src, path, backingSrc,
> + create_tool);
> + if (!cmd)
> + goto cleanup;
> +
> + if (virCommandRun(cmd, NULL) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +
> static int
> virStorageBackendCreateQemuImg(virConnectPtr conn,
> virStoragePoolObjPtr pool,
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index bb1e8d8..c591845 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -249,6 +249,11 @@ virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol);
> virStorageFileBackendPtr virStorageFileBackendForTypeInternal(int type,
> int protocol,
> bool report);
> +int
> +virStorageBackendCreateQemuImgFileFromSource(virStorageSourcePtr src,
> + const char *path,
> + virStorageSourcePtr backingSrc,
> + const char *create_tool);
>
>
> struct _virStorageFileBackend {
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 551a0ca..162e065 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2725,6 +2725,33 @@ virStorageFileCreate(virStorageSourcePtr src)
> return ret;
> }
>
> +/**
> + * virStorageFileCreate: Creates a storage file stub with the specified
> + * format
> + *
> + * @src: file structure pointing to the file
> + * @path: path to the file
> + * @backingSrc: file structure pointing to the backing file
> + * @create_tool: path to qemu-img
> + *
> + * Returns 0 on success, -2 if the function isn't supported by the backend,
> + * -1 on other failure. Errno is set in case of failure.
> + */
> +int virStorageFileCreateWithFormat(virStorageSourcePtr src,
> + const char *path,
> + virStorageSourcePtr backingSrc,
> + const char *create_tool)
> +{
> + if (!virStorageFileIsInitialized(src) ||
> + !src->drv->backend->storageFileCreate) {
> + errno = ENOSYS;
This check doesn't make sense since storageFileCreate is not used. The
function that calls qemu-img should make sure that qemu-img actually is
able to create the file.
> + return -2;
> + }
> +
> + return virStorageBackendCreateQemuImgFileFromSource(src, path, backingSrc,
> + create_tool);
> +}
> +
>
> /**
> * virStorageFileUnlink: Unlink storage file via storage driver
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list