On 08/26/2014 09:27 AM, Peter Krempa wrote:
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
s/suported/supported/
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(-)
+ 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);
This IS the normal path for rebase, all hidden in a (massively ugly) common routine, which...
Also this function frees vm internally.
...does some ugly things like transfer semantics on vm. I could do: if (flags & REBASE_COPY) { lots of indented code } else { /* normal rebase */ return qemuDomainBlockJobImpl() } but then the odd vm freeing semantics got harder, which is why I inverted it. So I think the best I can do is add more comments in v3.
+ 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.
Yes, and I consider it a maintenance fix for ease-of-understanding (I had it buggy in v1, and it was one of the hangs I had to debug). I'll call it out better in the commit message. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org