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(a)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