On Tue, Jun 2, 2015 at 6:00 PM, Victor Toso <victort...@redhat.com> wrote:
> PipeInputStream and PipeOutputStream should not fail when creating
> GPollableStream source as this currently does not work with default
> write_all and read_all functions;
>
> This patch removes the g_return_val_if_fail but keeps a g_debug in order
> to track problems;
>
> In order to avoid creating zombie GSource in create_source of both
> PipeInputStream and PipeOutputStream, we track all created GSources and
> set them to be dispatched when data is available to read/write. It is
> worth to mention that concurrent write/read is not possible with current
> giopipe and only the last created GSource will read the data as it is
> dispatched first.
> ---
> gtk/giopipe.c | 60
> +++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/gtk/giopipe.c b/gtk/giopipe.c
> index 50edb5b..cdfa070 100644
> --- a/gtk/giopipe.c
> +++ b/gtk/giopipe.c
> @@ -45,6 +45,7 @@ struct _PipeInputStream
> */
> gboolean peer_closed;
> GSource *source;
> + GList *created_sources;
> };
>
> struct _PipeInputStreamClass
> @@ -70,6 +71,7 @@ struct _PipeOutputStream
> gsize count;
> gboolean peer_closed;
> GSource *source;
> + GList *created_sources;
> };
>
> struct _PipeOutputStreamClass
> @@ -121,11 +123,26 @@ pipe_input_stream_read (GInputStream *stream,
> }
>
> static void
> +set_all_sources_ready (GList *sources)
> +{
> + GList *it;
> + for (it = sources; it != NULL; it = it->next) {
> + GSource *s = it->data;
> + if (s != NULL && !g_source_is_destroyed(s))
> + g_source_set_ready_time(s, 0);
> + }
> + g_list_free_full (sources, (GDestroyNotify) g_source_unref);
> +}
> +
> +static void
> pipe_input_stream_check_source (PipeInputStream *self)
> {
> if (self->source && !g_source_is_destroyed(self->source) &&
> -
> g_pollable_input_stream_is_readable(G_POLLABLE_INPUT_STREAM(self)))
> - g_source_set_ready_time(self->source, 0);
> +
> g_pollable_input_stream_is_readable(G_POLLABLE_INPUT_STREAM(self))) {
> + set_all_sources_ready(self->created_sources);
> + self->created_sources = NULL;
> + self->source = NULL;
>
Why do you free the sources? Only the destroyed one should be enough. The
rest should still remain "dispatchable"
> + }
> }
>
> static gboolean
> @@ -193,10 +210,9 @@ pipe_input_stream_dispose(GObject *object)
> self->peer = NULL;
> }
>
> - if (self->source) {
> - g_source_unref(self->source);
> - self->source = NULL;
> - }
> + g_list_free_full (self->created_sources, (GDestroyNotify)
> g_source_unref);
> + self->created_sources = NULL;
> + self->source = NULL;
>
> G_OBJECT_CLASS(pipe_input_stream_parent_class)->dispose (object);
> }
> @@ -234,14 +250,13 @@ 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);
> + if (self->source != NULL && !g_source_is_destroyed (self->source))
> + g_debug ("%s: GPollableSource already exists %p - This could lead
> to data loss (%ld)",
> + G_STRFUNC, self->source, self->read);
>
You may remove this debug now that you track all created sources
>
> pollable_source = g_pollable_source_new_full (self, NULL,
> cancellable);
> self->source = g_source_ref (pollable_source);
> + self->created_sources = g_list_prepend (self->created_sources,
> self->source);
>
furthermore, you may just get rid of self->source, it's just one of the
many now.
then, I think "created_sources" should be named simply "sources"
> return pollable_source;
> }
> @@ -319,10 +334,9 @@ pipe_output_stream_dispose(GObject *object)
> self->peer = NULL;
> }
>
> - if (self->source) {
> - g_source_unref(self->source);
> - self->source = NULL;
> - }
> + g_list_free_full (self->created_sources, (GDestroyNotify)
> g_source_unref);
> + self->created_sources = NULL;
> + self->source = NULL;
>
> G_OBJECT_CLASS(pipe_output_stream_parent_class)->dispose (object);
> }
> @@ -331,8 +345,11 @@ static void
> pipe_output_stream_check_source (PipeOutputStream *self)
> {
> if (self->source && !g_source_is_destroyed(self->source) &&
> -
> g_pollable_output_stream_is_writable(G_POLLABLE_OUTPUT_STREAM(self)))
> - g_source_set_ready_time(self->source, 0);
> +
> g_pollable_output_stream_is_writable(G_POLLABLE_OUTPUT_STREAM(self))) {
> + set_all_sources_ready(self->created_sources);
> + self->created_sources = NULL;
> + self->source = NULL;
> + }
> }
>
> static gboolean
> @@ -416,14 +433,13 @@ 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);
> + if (self->source != NULL && !g_source_is_destroyed (self->source))
> + g_debug ("%s: GPollableSource already exists %p - This could lead
> to data loss (%ld)",
> + G_STRFUNC, self->source, self->count);
>
> pollable_source = g_pollable_source_new_full (self, NULL,
> cancellable);
> self->source = g_source_ref (pollable_source);
> + self->created_sources = g_list_prepend (self->created_sources,
> self->source);
>
>
idem
> return pollable_source;
> }
> --
> 2.4.2
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-
> <http://lists.freedesktop.org/mailman/listinfo/spice-devel>
devel <http://lists.freedesktop.org/mailman/listinfo/spice-devel>
>
looks good otherwise
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
