Re: [libvirt] [PATCH 2/7] Support callbacks on virStream APIs in remote driver client
On Thu, Aug 19, 2010 at 02:14:58PM -0600, Eric Blake wrote:
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote:
> The current remote driver code for streams only supports
> blocking I/O mode. This is fine for the usage with migration
> but is a problem for more general use cases, in particular
> bi-directional streams.
>
> This adds supported for the stream callbacks and non-blocking
> I/O. with the minor caveat is that it doesn't actually do
> non-blocking I/O for sending stream data, only receiving it.
> A future patch will try todo non-blocking sends, but this is
s/todo/to do/
> quite tricky to get right.
>
> * src/remote/remote_driver.c: Allow non-blocking I/O for
> streams and support callbacks
> ---
> src/remote/remote_driver.c | 188 ++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 172 insertions(+), 16 deletions(-)
>
> +static void
> +remoteStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque)
> +{
> + virStreamPtr st = opaque;
> + struct private_data *priv = st->conn->privateData;
> + struct private_stream_data *privst = st->privateData;
> +
> + remoteDriverLock(priv);
> + if (privst->cb &&
> + (privst->cbEvents & VIR_STREAM_EVENT_READABLE) &&
> + privst->incomingOffset) {
> + virStreamEventCallback cb = privst->cb;
> + void *cbOpaque = privst->cbOpaque;
> + virFreeCallback cbFree = privst->cbFree;
> +
> + 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
> + 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 :-)
> + (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.
Also, any reason you use (cb)() instead of the simpler cp() calling
convention?
No particular reason.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|