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

[Marc-André Lureau]

On Thu, Feb 6, 2014 at 10:45 AM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 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.

ok, it can be splitted apart, since it is some refactoring from
spice-port. I forgot that.

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

Something like: "Feed a SpiceVmc stream with new data from a coroutine
context."?

>
>> >> +    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? I can imagine it makes sense, and it
seems doable, but I don't see that as a hard requirement, rather a
small improvement.


-- 
Marc-André Lureau