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

Re: [libvirt] [PATCH v2 5/8] blockcopy: tweak how rebase calls into copy



On 08/26/14 13:21, 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.  Furthermore, the existing
> semantics of allocating vm in one function and freeing it in
> another is awkward to work with, but the helper function has
> to be able to tell the caller if the domain unexpectedly died
> while locks were dropped for running a monitor command.
> 
> * 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 | 104 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 61 insertions(+), 43 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ad75bd9..f3c5387 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

...

> @@ -15461,6 +15458,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
>                        unsigned long bandwidth, unsigned int flags)
>  {
>      virDomainObjPtr vm;
> +    int ret = -1;
> +    virStorageSourcePtr dest = NULL;
> 
>      virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
>                    VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
> @@ -15476,17 +15475,36 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
>          return -1;
>      }
> 
> -    if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY) {
> -        const char *format = NULL;
> -        if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW)
> -            format = "raw";
> -        flags &= ~(VIR_DOMAIN_BLOCK_REBASE_COPY |
> -                   VIR_DOMAIN_BLOCK_REBASE_COPY_RAW);
> -        return qemuDomainBlockCopy(vm, dom->conn, path, base, format, bandwidth, flags);
> +    if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY))

This inversion of logic here isn't intuitive. I'd rather see the normal
path to call into stuff necessary to do a block rebase and a special
case for block copy.

> +        return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth,
> +                                      NULL, BLOCK_JOB_PULL, flags);

Also this function frees vm internally.

> +
> +    if (VIR_ALLOC(dest) < 0)
> +        goto cleanup;
> +    dest->type = VIR_STORAGE_TYPE_FILE;
> +    if (VIR_STRDUP(dest->path, base) < 0)
> +        goto cleanup;
> +    if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW)
> +        dest->format = VIR_STORAGE_FILE_RAW;
> +    flags &= ~(VIR_DOMAIN_BLOCK_REBASE_COPY |
> +               VIR_DOMAIN_BLOCK_REBASE_COPY_RAW);
> +    if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) {
> +        /* FIXME: should this check be in libvirt.c? */
> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                       _("Relative rebase incompatible with block copy"));
> +        goto cleanup;
>      }
> +    /* We rely on the fact that VIR_DOMAIN_BLOCK_REBASE_SHALLOW
> +     * and VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT map to the same value
> +     * as for block copy. */
> +    ret = qemuDomainBlockCopyCommon(&vm, dom->conn, path, dest,
> +                                    bandwidth, flags);
> 
> -    return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth, NULL,
> -                                  BLOCK_JOB_PULL, flags);
> + cleanup:
> +    if (vm)

While here the caller is responsible.

> +        virObjectUnlock(vm);
> +    virStorageSourceFree(dest);
> +    return ret;
>  }
> 
>  static int
> 

Feels a bit messy although it's functionally correct.

ACK

Peter


Attachment: signature.asc
Description: OpenPGP digital signature


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