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@xxxxxxxxxx +1-801-349-2682 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