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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org