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

[Marc-André Lureau]

The patch was missing a second case where channels are removed from
the session, see attached patch.

Fundamentally, it doesn't change much from my patch, I guess removing
g_clear_object(channel->priv->session) and ignoring the case where the
channel isn't in the session (like you do here) would have been the
same.

I still think it's a better idea to clear the channel->session if the
channel has been disconnect from the session, although I agree that it
requires a bit more testing and fixing for some warnings/criticals.

On Fri, Nov 21, 2014 at 11:06 AM, Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
> 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



-- 
Marc-André Lureau
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-channels-from-SpiceSession-channels-on-sessio.patch
Type: text/x-patch
Size: 3582 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20141121/07e21940/attachment.bin>