On 08/31/14 06:02, Eric Blake wrote:
> In order to implement the new virDomainBlockCopy, the existing
> block copy internal implementation needs to be adjusted. The
> new function will parse XML into a storage source, and parse
> typed parameters into integers, then call into the same common
> backend. For now, it's easier to keep the same implementation
> limits that only local file destinations are suported, but now
> the check needs to be explicit.
>
> * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Rename...
> (qemuDomainBlockCopyCommon): ...and adjust parameters.
> (qemuDomainBlockRebase): Adjust caller.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 108 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 58 insertions(+), 50 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 946c384..d3f1042 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15351,11 +15351,12 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path,
>
> /* bandwidth in bytes/s */
> static int
> -qemuDomainBlockCopy(virDomainObjPtr vm,
> - virConnectPtr conn,
> - const char *path,
> - const char *dest, const char *format,
> - unsigned long long bandwidth, unsigned int flags)
You should mention that this function consumes @dest.
> +qemuDomainBlockCopyCommon(virDomainObjPtr vm,
> + virConnectPtr conn,
> + const char *path,
> + virStorageSourcePtr dest,
> + unsigned long long bandwidth,
> + unsigned int flags)
> {
> virQEMUDriverPtr driver = conn->privateData;
> qemuDomainObjPrivatePtr priv;
> @@ -15367,11 +15368,12 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
> bool need_unlink = false;
> virStorageSourcePtr mirror = NULL;
> virQEMUDriverConfigPtr cfg = NULL;
> + const char *format = NULL;
> + int desttype = virStorageSourceGetActualType(dest);
>
> /* Preliminaries: find the disk we are editing, sanity checks */
> - virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
> - VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
> - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1);
> + virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
> + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1);
>
> priv = vm->privateData;
> cfg = virQEMUDriverGetConfig(driver);
> @@ -15416,8 +15418,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
> if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> goto endjob;
>
> - if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) &&
> - STREQ_NULLABLE(format, "raw") &&
> + if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) &&
> + dest->format == VIR_STORAGE_FILE_RAW &&
> disk->src->backingStore->path) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("disk '%s' has backing file, so raw shallow copy "
> @@ -15427,72 +15429,68 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
> }
>
> /* Prepare the destination file. */
> - if (stat(dest, &st) < 0) {
> + /* XXX Allow non-file mirror destinations */
> + if (!virStorageSourceIsLocalStorage(dest)) {
> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("non-file destination not supported yet"));
> + }
> + if (stat(dest->path, &st) < 0) {
> if (errno != ENOENT) {
> virReportSystemError(errno, _("unable to stat for disk %s: %s"),
> - disk->dst, dest);
> + disk->dst, dest->path);
> goto endjob;
> - } else if (flags & (VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
> - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV)) {
> + } else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT ||
> + desttype == VIR_STORAGE_TYPE_BLOCK) {
> virReportSystemError(errno,
> _("missing destination file for disk %s: %s"),
> - disk->dst, dest);
> + disk->dst, dest->path);
> goto endjob;
> }
> } else if (!S_ISBLK(st.st_mode)) {
> - if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) {
> + if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("external destination file for disk %s already "
> "exists and is not a block device: %s"),
> - disk->dst, dest);
> + disk->dst, dest->path);
> goto endjob;
> }
> - if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) {
> + if (desttype == VIR_STORAGE_TYPE_BLOCK) {
> virReportError(VIR_ERR_INVALID_ARG,
> _("blockdev flag requested for disk %s, but file "
> - "'%s' is not a block device"), disk->dst, dest);
> + "'%s' is not a block device"),
> + disk->dst, dest->path);
> goto endjob;
> }
> }
>
> - if (VIR_ALLOC(mirror) < 0)
> + if (!(mirror = virStorageSourceCopy(dest, false)))
Also you won't need to copy it then.
> goto endjob;
> - /* XXX Allow non-file mirror destinations */
> - mirror->type = flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV ?
> - VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE;
>
> - if (format) {
> - if ((mirror->format = virStorageFileFormatTypeFromString(format)) <= 0) {
> - virReportError(VIR_ERR_INVALID_ARG, _("unrecognized format '%s'"),
> - format);
> - goto endjob;
> - }
> - } else {
> - if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) {
> + if (!dest->format) {
> + if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
> mirror->format = disk->src->format;
> } else {
> /* If the user passed the REUSE_EXT flag, then either they
> - * also passed the RAW flag (and format is non-NULL), or it is
> - * safe for us to probe the format from the file that we will
> - * be using. */
> - mirror->format = virStorageFileProbeFormat(dest, cfg->user,
> + * can also pass the RAW flag or use XML to tell us the format.
> + * So if we get here, we assume it is safe for us to probe the
> + * format from the file that we will be using. */
> + mirror->format = virStorageFileProbeFormat(dest->path, cfg->user,
> cfg->group);
> }
> }
>
> /* pre-create the image file */
> - if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) {
> - int fd = qemuOpenFile(driver, vm, dest, O_WRONLY | O_TRUNC | O_CREAT,
> + if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
> + int fd = qemuOpenFile(driver, vm, dest->path,
> + O_WRONLY | O_TRUNC | O_CREAT,
> &need_unlink, NULL);
> if (fd < 0)
> goto endjob;
> VIR_FORCE_CLOSE(fd);
> }
>
> - if (!format && mirror->format > 0)
> + if (mirror->format > 0)
> format = virStorageFileFormatTypeToString(mirror->format);
> - if (VIR_STRDUP(mirror->path, dest) < 0)
> - goto endjob;
>
> if (virStorageSourceInitChainElement(mirror, disk->src, false) < 0)
> goto endjob;
> @@ -15506,8 +15504,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
>
> /* Actually start the mirroring */
> qemuDomainObjEnterMonitor(driver, vm);
> - ret = qemuMonitorDriveMirror(priv->mon, device, dest, format, bandwidth,
> - flags);
> + ret = qemuMonitorDriveMirror(priv->mon, device, dest->path, format,
> + bandwidth, flags);
> virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);
> qemuDomainObjExitMonitor(driver, vm);
> if (ret < 0) {
> @@ -15527,8 +15525,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
> vm->def->name);
>
> endjob:
> - if (need_unlink && unlink(dest))
> - VIR_WARN("unable to unlink just-created %s", dest);
> + if (need_unlink && unlink(dest->path))
> + VIR_WARN("unable to unlink just-created %s", dest->path);
> virStorageSourceFree(mirror);
> if (!qemuDomainObjEndJob(driver, vm))
> vm = NULL;
> @@ -15546,9 +15544,9 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
> unsigned long bandwidth, unsigned int flags)
> {
> virDomainObjPtr vm;
> - const char *format = NULL;
> int ret = -1;
> unsigned long long speed = bandwidth;
> + virStorageSourcePtr dest = NULL;
>
> virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
> VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
> @@ -15570,8 +15568,14 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
> BLOCK_JOB_PULL, flags);
>
> /* If we got here, we are doing a block copy rebase. */
> + if (VIR_ALLOC(dest) < 0)
> + goto cleanup;
> + dest->type = (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) ?
> + VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE;
> + if (VIR_STRDUP(dest->path, base) < 0)
> + goto cleanup;
> if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW)
> - format = "raw";
> + dest->format = VIR_STORAGE_FILE_RAW;
>
> /* Convert bandwidth MiB to bytes */
> if (speed > LLONG_MAX >> 20) {
> @@ -15591,15 +15595,19 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
> goto cleanup;
> }
>
> + /* We rely on the fact that VIR_DOMAIN_BLOCK_REBASE_SHALLOW
> + * and VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT map to the same values
> + * as for block copy. */
> flags &= (VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
> - VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
> - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV);
> - ret = qemuDomainBlockCopy(vm, dom->conn, path, base, format,
> - bandwidth, flags);
> + VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT);
> + ret = qemuDomainBlockCopyCommon(vm, dom->conn, path, dest,
> + bandwidth, flags);
> vm = NULL;
> +
> cleanup:
> if (vm)
> virObjectUnlock(vm);
> + virStorageSourceFree(dest);
If you don't free it afterwards.
> return ret;
> }
>
ACK with the docs added. The tweaks of the @dest handling are not necessary.
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list