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@xxxxxxxxxx>
>> ---
>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list