[libvirt] [PATCH v2 5/8] blockcopy: tweak how rebase calls into copy

[Eric Blake]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140826/5db22b4c/attachment-0001.sig>