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?
> + 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)'.
> + (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.
Also, any reason you use (cb)() instead of the simpler cp() calling
convention?
> static int
> -remoteStreamEventRemoveCallback(virStreamPtr stream ATTRIBUTE_UNUSED)
> +remoteStreamEventRemoveCallback(virStreamPtr st)
> {
> - return -1;
> + struct private_data *priv = st->conn->privateData;
> + struct private_stream_data *privst = st->privateData;
> + int ret = -1;
> +
> + remoteDriverLock(priv);
> +
> + if (!privst->cb) {
> + remoteError(VIR_ERR_INTERNAL_ERROR,
> + _("no stream callback registered"));
> + goto cleanup;
> + }
> +
> + if (!privst->cbDispatch &&
> + privst->cbFree)
> + (privst->cbFree)(privst->cbOpaque);
Depending on whether you feel the callback should be run without the
driver lock, this might need changing.
--
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