[Spice-devel] [PATCH spice-gtk 15/15] session: disconnect in idle

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20141126/f2303c35/attachment-0001.sig>