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

[Christophe Fergeau]

On Wed, Feb 05, 2014 at 05:42:10PM +0100, Marc-André Lureau wrote:
> On Wed, Feb 5, 2014 at 5:01 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > On Wed, Jan 22, 2014 at 07:26:50PM +0100, Marc-André Lureau wrote:
> >> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> >>
> >> This allows to use conveniently GIOStream APIs without caring about
> >> coroutine and Spice messages details.
> >> ---
> >
> > I think Hans asked you to split these helpers in a separate commit in his
> > review.
> >
> 
> I don't see any good reason to do that.

Hans took time to review your patches, made some comments, it would be nice
to give him an answer rather than being silent about this on patch resend..
With that said, these changes indeed seem self contained, and if they cause
issues, having git annotate, git bisect, ... point at a small patch is
much easier to handle than getting a reference to a big commit.


> >> diff --git a/gtk/vmcstream.c b/gtk/vmcstream.c
> >> new file mode 100644
> >> index 0000000..724ee58
> >> --- /dev/null
> >> +++ b/gtk/vmcstream.c
> >> @@ -0,0 +1,532 @@
> >> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> >> +/*
> >> +  Copyright (C) 2013 Red Hat, Inc.
> >
> > 2014
> >
> 
> Thanks for being extra picky, but how is this important? It was mostly
> written in 2013.

You did not mention anywhere in the patch that it's a resend, I did not
realize you already sent previous versions of this patch before you told
me. 2013, 2014 works too, or just 2013 if you really did not make any
changes in 2014.

> >> +
> >> +static void
> >> +spice_vmc_input_stream_init(SpiceVmcInputStream *self)
> >> +{
> >> +}
> >> +
> >> +static SpiceVmcInputStream *
> >> +spice_vmc_input_stream_new(void)
> >> +{
> >> +    SpiceVmcInputStream *self;
> >> +
> >> +    self = g_object_new(SPICE_TYPE_VMC_INPUT_STREAM, NULL);
> >> +
> >> +    return self;
> >> +}
> >> +
> >> +/* 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.


> >> +    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.

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/11639235/attachment.pgp>