Re: [libvirt] [PATCH v2 5/8] blockcopy: tweak how rebase calls into copy
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(a)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
Attachments:
- signature.asc (application/pgp-signature — 539 bytes)