[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 14/18] blockcopy: tweak how rebase calls into copy



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 redhat com>
> ---
>  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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]