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@xxxxxxxxxx>
> ---
> 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list