[Spice-devel] [spice-gtk PATCH v2 1/5] giopipe: don't fail on create_source

[Victor Toso]

Hi,

On Wed, May 20, 2015 at 12:51:46PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, May 19, 2015 at 4:34 PM, Victor Toso <victortoso at redhat.com> wrote:
> 
> > PipeInputStream and PipeOutputStream should not fail when creating
> > GPollableStream source. It is already checked and unref in case of
> > existing source.
> > ---
> >  gtk/giopipe.c | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/gtk/giopipe.c b/gtk/giopipe.c
> > index 440cae9..32fa4fa 100644
> > --- a/gtk/giopipe.c
> > +++ b/gtk/giopipe.c
> > @@ -234,9 +234,6 @@ pipe_input_stream_create_source (GPollableInputStream
> > *stream,
> >      PipeInputStream *self = PIPE_INPUT_STREAM(stream);
> >      GSource *pollable_source;
> >
> > -    g_return_val_if_fail (self->source == NULL ||
> > -                          g_source_is_destroyed (self->source), NULL);
> > -
> >      if (self->source && g_source_is_destroyed (self->source))
> >          g_source_unref (self->source);
> >
> > @@ -416,9 +413,6 @@ pipe_output_stream_create_source
> > (GPollableOutputStream *stream,
> >      PipeOutputStream *self = PIPE_OUTPUT_STREAM(stream);
> >      GSource *pollable_source;
> >
> > -    g_return_val_if_fail (self->source == NULL ||
> > -                          g_source_is_destroyed (self->source), NULL);
> > -
> >      if (self->source && g_source_is_destroyed (self->source))
> >          g_source_unref (self->source);
> >
>
> The check was also preventing concurrent read/write. Without this check, it
> is possible to have several "active" sources on the same stream. However,
> it will notify only the last reader/writer. With your test though, the
> operations are sequential, the old source is destroyed when the loop ends
> the dispatch(). However, to avoid leaks, the  if (self->source &&
> g_source_is_destroyed (self->source)) check must be changed to if
> (self->source).
>

I created the test with sequential operations because this is how it
works with channel-webdav. Just to be sure, I created a test with with
concurrent write and we get G_IO_ERROR_PENDING as error. So, regarding
concurrent write I believe we are safe.

> This is potentially dangerous change. I would be okay with it, but it needs
> a strong warning, perhaps a debug() message. Other solution is to track all
> sources and notify the one that are not destroyed.

I've applied your suggestions plus the concurrent test, sending it now.

Many thanks for the explanation and review,
Victor Toso