[Spice-devel] [spice-gtk] Remove channels from SpiceSession::channels on session disconnect

[Marc-André Lureau]

It doesn't work well, I get a crash with pretty poor backtrace when
migrating with virt-manager (looks like an "extra" unref from python
GC).

On Thu, Nov 20, 2014 at 3:52 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> spice_session_disconnect gets rid of the reference it owns on
> the SpiceChannels listed in SpiceSession::channels. However, it does
> not remove them from that list, which would cause multiple calls to
> spice_session_disconnect to remove references it doesn't own, which
> will cause memory management issues later on.
>
> This patch removes the channels from SpiceSession::channels after they
> are unref'ed to ensure the reference SpiceSession owns will not be
> removed multiple times.
>
> It would be cleaner to clear the SpiceChannel::session when it's removed
> from the session, but client applications (virt-viewer)/other spice-gtk
> code do not deal well with SpiceChannel::session getting set to NULL
> when a disconnect event happens.
> ---
> Hey,
>
> I've experimented with that as well, and it seems that this patch would solve
> that issue with using spice-gtk from python/... (haven't tested virt-manager at
> all), and would cause less issues with existing code, what do you think?
>
> Christophe
>
>
>  gtk/spice-session.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index c44a3e1..df71eb7 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -1650,8 +1650,12 @@ void spice_session_disconnect(SpiceSession *session)
>      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_channel_destroy(item->channel); /* /!\ item and channel are destroy() after this call */
> +        ring_remove(&item->link);
> +        spice_channel_disconnect(item->channel, SPICE_CHANNEL_NONE);
> +        g_object_unref(item->channel);
> +        free(item);
>      }
> +    g_warn_if_fail(ring_is_empty(&s->channels));
>
>      s->connection_id = 0;
>
> @@ -1940,7 +1944,7 @@ void spice_session_channel_destroy(SpiceSession *session, SpiceChannel *channel)
>  {
>      SpiceSessionPrivate *s = session->priv;
>      struct channel *item = NULL;
> -    RingItem *ring;
> +    RingItem *ring = NULL;
>
>      g_return_if_fail(s != NULL);
>      g_return_if_fail(channel != NULL);
> @@ -1955,15 +1959,16 @@ void spice_session_channel_destroy(SpiceSession *session, SpiceChannel *channel)
>              break;
>      }
>
> -    g_return_if_fail(ring != NULL);
>
>      if (channel == s->cmain) {
>          CHANNEL_DEBUG(channel, "the session lost the main channel");
>          s->cmain = NULL;
>      }
>
> -    ring_remove(&item->link);
> -    free(item);
> +    if (ring != NULL) {
> +        ring_remove(&item->link);
> +        free(item);
> +    }
>
>      g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_DESTROY], 0, channel);
>  }
> --
> 2.1.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau