[Spice-devel] [spice-gtk 4/5] Add SpiceVMC GIOStream

[Christophe Fergeau]

On Thu, Feb 06, 2014 at 11:00:44AM +0100, Marc-André Lureau wrote:
> >> >> +
> >> >> +/* coroutine */
> >> >> +G_GNUC_INTERNAL void
> >> >> +spice_vmc_input_stream_co_data(SpiceVmcInputStream *self,
> >> >> +                               const gpointer d, gsize size)
> >> >
> >> > This function is not used in this patch, is unusual in a stream
> >> > implementation, ... it's probably better to add it with the webdav
> >> > patch or in a separate commit, and to have a comment explaining what
> >> > it is.
> >>
> >> It is necessary to feed the stream from a coroutine. Without it, you
> >> won't get any data from input stream.
> >
> > Some description of the purpose of this function, how the async machinery
> > work with coroutines would be useful as a code comment.
> 
> Something like: "Feed a SpiceVmc stream with new data from a coroutine
> context."?

Yup, plus explanations that the once an _async() operation on the stream
has been initiated, it won't return before the corresponding coroutine
calls spice_vmc_input_stream_co_data() to notify it that some data is available.

> 
> >
> >> >> +    self->all = TRUE;
> >> >> +    self->buffer = buffer;
> >> >> +    self->count = count;
> >> >> +    self->pos = 0;
> >> >> +    result = g_simple_async_result_new(G_OBJECT(self),
> >> >> +                                       callback,
> >> >> +                                       user_data,
> >> >> +                                       spice_vmc_input_stream_read_async);
> >> >
> >> > I think it's missing
> >> >        g_simple_async_result_set_op_res_gssize(self->result, count);
> >> >
> >>
> >> it's done when we know what to return, in spice_vmc_input_stream_co_data()
> >>
> >> >> +    self->result = result;
> >> >
> >> > Calling _finish() is not supposed to be mandatory after calling an _async()
> >> > function. Things will not work quite well here if _finish() is not called,
> >> > it would be nice to at least document it.
> >> >
> >>
> >> You don't "have" to.
> >> and when data comes, it's calling g_simple_async_result_complete_in_idle()
> >>
> >> so things works the way I  needed to.
> >
> > As far as I can tell, self->cancellable will never be freed/set to NULL
> > if you don't call _finish(), so spice_vmc_input_stream_read_async can't be
> > called again unless you called _finish() first.
> 
> Oh ok, will try to improve that a bit. Btw, where did you see that not
> calling _finish() is not a bug?


This is coming from
https://developer.gnome.org/gio/stable/GAsyncResult.html
"If the result or error status of the operation is not needed, there is no
need to call the "_finish()" function; GIO will take care of cleaning up
the result and error information after the GAsyncReadyCallback returns. You
can pass NULL for the GAsyncReadyCallback if you don't need to take any
action at all after the operation completes."


> I can imagine it makes sense, and it
> seems doable, but I don't see that as a hard requirement, rather a
> small improvement.

It's only internal code, so just a comment explaining this difference would
be enough imo
/* Contrary to most GIO async methods, _finish() has to be called before
 * doing more async() calls. _finish() also cleans up data used for the async
 * call.
 */

I agree it's not very important to spend time on the code to get this
perfect.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140206/d2c590fe/attachment.pgp>