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 <[email protected]> > --- > 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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
