[libvirt] [PATCH 2/7] Support callbacks on virStream APIs in remote driver client

[Eric Blake]

On 08/20/2010 03:07 AM, Daniel P. Berrange wrote:
>>> +        privst->cbDispatch = 1;
>>> +        remoteDriverUnlock(priv);
>>> +        (cb)(st, VIR_STREAM_EVENT_READABLE, cbOpaque);
>>
>> Do we have/want a return value from this callback?  If so, what would we
>> do about a non-zero return value?
> 
> You could have a boolean return value to indicate whether the
> callback should be removed or not. I find that pattern a little
> confusing though, because I can never remember whether 'true'
> or 'false' mean remove or keep. Since we run unlocked, the
> callback can explicitly remove itself by calling into the APIs

Thus, a boolean is the wrong choice; that would argue that if the
callback returns anything, it should be an enum value, where the name
implies the desired action.  I guess I'm thinking more of an iterator
callback, where it is handy to return a non-zero value to abort
iterating over further elements.

On the other hand, it is always harder to add a return value down the
road if we come up with a compelling reason, if it requires changing
signatures and existing callbacks; is it worth having a return value
(and ignoring it) now, to allow for easier use of such a value in the
future?  But that's not a reason to reject this patch.

> 
>>> +        remoteDriverLock(priv);
>>> +        privst->cbDispatch = 0;
>>> +
>>> +        if (!privst->cb && cbFree)
>>
>> Can never be true - privst->cb had to be non-NULL 12 lines earlier to
>> get to this point.  I think you meant just 'if (cbFree)'.
> 
> Remember we just unlocked the driver. If the callback had invoked
> virStreamRemoveCallback, then privst->cb is now NULL. If we see
> this condition, then we have to now free the data. Re-entrancy is
> fun :-)

Oh.  Thanks for pointing that out.  You're correct; no change needed.

> 
>>> +            (cbFree)(cbOpaque);
>>
>> Any reason that cp is called while the driver is unlocked, but cbFree is
>> called while the lock is still held?  It seems like if we are worried
>> that the callbacks might deadlock if we still hold the driver lock, then
>> we should treat both callbacks in the same manner.
> 
> The main callback is very likely to call back into libvirt. The free
> callback's only purpose is to release memory, so there is no reasonable
> expectation of it calling back into libvirt.

Should we document that as part of the contract of the cbFree callback?

At any rate, you've answered my questions, so:

ACK, with the spelling nit in the commit message resolved.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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