On Tue, Nov 25, 2014 at 02:19:28PM +0100, Marc-André Lureau wrote:
> This is a workaround for existing clients such as virt-viewer that do
> not hold a reference to their sessions when calling
> spice_session_disconnect() and crash now that channels are removed from
> session during the call. They expect disconnection events to be deferred
> instead, let's defer actual disconnection to idle time for public
> disconnect API for compatibility reasons (it is still recommended to fix
> client code, for eventual future iterations)
> ---
> gtk/spice-session.c | 72 +++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 45 insertions(+), 27 deletions(-)
>
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index e00e2b3..300faf5 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -258,6 +258,31 @@ static void spice_session_init(SpiceSession *session)
> }
>
> static void
> +session_disconnect(SpiceSession *self)
> +{
> + SpiceSessionPrivate *s;
> + struct channel *item;
> + RingItem *ring, *next;
> +
> + s = self->priv;
> + s->cmain = NULL;
> +
> + for (ring = ring_get_head(&s->channels); ring != NULL; ring = next) {
> + next = ring_next(&s->channels, ring);
> + item = SPICE_CONTAINEROF(ring, struct channel, link);
> + spice_session_channel_destroy(self, item->channel);
> + }
> +
> + s->connection_id = 0;
> +
> + g_free(s->name);
> + s->name = NULL;
> + memset(s->uuid, 0, sizeof(s->uuid));
> +
> + spice_session_abort_migration(self);
> +}
> +
> +static void
> spice_session_dispose(GObject *gobject)
> {
> SpiceSession *session = SPICE_SESSION(gobject);
> @@ -265,11 +290,12 @@ spice_session_dispose(GObject *gobject)
>
> SPICE_DEBUG("session dispose");
>
> - spice_session_disconnect(session);
> + session_disconnect(session);
>
> g_warn_if_fail(s->migration == NULL);
> g_warn_if_fail(s->migration_left == NULL);
> g_warn_if_fail(s->after_main_init == 0);
> + g_warn_if_fail(s->disconnecting == 0);
>
> g_clear_object(&s->audio_manager);
> g_clear_object(&s->usb_manager);
> @@ -1384,7 +1410,7 @@ gboolean spice_session_connect(SpiceSession *session)
> s = session->priv;
> g_return_val_if_fail(!s->disconnecting, FALSE);
>
> - spice_session_disconnect(session);
> + session_disconnect(session);
>
> s->client_provided_sockets = FALSE;
>
> @@ -1419,7 +1445,7 @@ gboolean spice_session_open_fd(SpiceSession *session, int fd)
> s = session->priv;
> g_return_val_if_fail(!s->disconnecting, FALSE);
>
> - spice_session_disconnect(session);
> + session_disconnect(session);
>
> s->client_provided_sockets = TRUE;
>
> @@ -1576,7 +1602,7 @@ void spice_session_abort_migration(SpiceSession *session)
> end:
> g_list_free(s->migration_left);
> s->migration_left = NULL;
> - spice_session_disconnect(s->migration);
> + session_disconnect(s->migration);
> g_object_unref(s->migration);
> s->migration = NULL;
>
> @@ -1616,7 +1642,7 @@ void spice_session_channel_migrate(SpiceSession *session, SpiceChannel *channel)
>
> if (g_list_length(s->migration_left) == 0) {
> CHANNEL_DEBUG(channel, "migration: all channel migrated, success");
> - spice_session_disconnect(s->migration);
> + session_disconnect(s->migration);
> g_object_unref(s->migration);
> s->migration = NULL;
> spice_session_set_migration_state(session, SPICE_SESSION_MIGRATION_NONE);
> @@ -1719,6 +1745,18 @@ gboolean spice_session_get_read_only(SpiceSession *self)
> return self->priv->read_only;
> }
>
> +static gboolean session_disconnect_idle(SpiceSession *self)
> +{
> + SpiceSessionPrivate *s = self->priv;
> +
> + session_disconnect(self);
> + s->disconnecting = 0;
> +
> + g_object_unref(self);
> +
> + return FALSE;
I'm trying to use return G_SOURCE_REMOVE; these days, but FALSE is fine too.
> +}
> +
> /**
> * spice_session_disconnect:
> * @session:
> @@ -1728,37 +1766,17 @@ gboolean spice_session_get_read_only(SpiceSession *self)
> void spice_session_disconnect(SpiceSession *session)
> {
> SpiceSessionPrivate *s;
> - struct channel *item;
> - RingItem *ring, *next;
>
> g_return_if_fail(SPICE_IS_SESSION(session));
> - g_return_if_fail(session->priv != NULL);
>
> s = session->priv;
>
> SPICE_DEBUG("session: disconnecting %d", s->disconnecting);
> - if (s->disconnecting)
> + if (s->disconnecting != 0)
> return;
>
> g_object_ref(session);
> - s->disconnecting = TRUE;
> - s->cmain = NULL;
> -
> - for (ring = ring_get_head(&s->channels); ring != NULL; ring = next) {
> - next = ring_next(&s->channels, ring);
> - item = SPICE_CONTAINEROF(ring, struct channel, link);
> - spice_session_channel_destroy(session, item->channel);
> - }
> -
> - s->connection_id = 0;
> -
> - g_free(s->name);
> - s->name = NULL;
> - memset(s->uuid, 0, sizeof(s->uuid));
> -
> - spice_session_abort_migration(session);
> - s->disconnecting = FALSE;
> - g_object_unref(session);
> + s->disconnecting = g_idle_add((GSourceFunc)session_disconnect_idle, session);
Since _dispose calls session_disconnect() anyway, we don't really need
to keep a ref on the session, and could just call g_source_remove() in
_dispose if s->disconnecting is not 0. Fine with me either way.
ACK.
Christophe
Attachment:
pgpkss3TkEsuk.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel