Re: [libvirt] [PATCH 2/7] Support callbacks on virStream APIs in remote driver client
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)