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