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

[Marc-André Lureau]

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

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.



-- 
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150520/9735acf0/attachment.html>