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

[Marc-André Lureau]

On Tue, Nov 18, 2014 at 4:02 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Fri, Nov 14, 2014 at 12:32:42AM +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)
>
> I'm afraid these changes are causing more issues. For example,
> virt-viewer --reconnect + virsh destroy expects the session to stay
> alive long enough, I needed this in order to fix it
>

What you mean by "these changes"? I suspect you mean starting from
"session: remove channels on disconnect". Can we already push the rest
of the changes proposed here?

> commit 78a45c4236fee24eeaf4638cc00ea5fda4e22318
> Author: Christophe Fergeau <cfergeau at redhat.com>
> Date:   Tue Nov 18 15:38:33 2014 +0100
>
>     keep session alive

This doesn't really explain what is happening. So I can't tell if it's
virt-viewer programming error or not.

>
> diff --git a/src/virt-viewer-display.c b/src/virt-viewer-display.c
> index e6bc108..a42276e 100644
> --- a/src/virt-viewer-display.c
> +++ b/src/virt-viewer-display.c
> @@ -93,6 +93,16 @@ enum {
>  };
>
>  static void
> +virt_viewer_display_finalize(GObject *obj)
> +{
> +    VirtViewerDisplay *display = VIRT_VIEWER_DISPLAY(obj);
> +    if (display->priv->session != NULL)
> +        g_clear_object(&display->priv->session);
> +
> +    G_OBJECT_CLASS(virt_viewer_display_parent_class)->finalize(obj);
> +}
> +
> +static void
>  virt_viewer_display_class_init(VirtViewerDisplayClass *class)
>  {
>      GObjectClass *object_class = G_OBJECT_CLASS(class);
> @@ -100,6 +110,7 @@ virt_viewer_display_class_init(VirtViewerDisplayClass *class)
>
>      object_class->set_property = virt_viewer_display_set_property;
>      object_class->get_property = virt_viewer_display_get_property;
> +    object_class->finalize = virt_viewer_display_finalize;
>
>  #if GTK_CHECK_VERSION(3, 0, 0)
>      widget_class->get_preferred_width = virt_viewer_display_get_preferred_width;
> @@ -316,7 +327,7 @@ virt_viewer_display_set_property(GObject *object,
>          break;
>      case PROP_SESSION:
>          g_warn_if_fail(priv->session == NULL);
> -        priv->session = g_value_get_object(value);
> +        priv->session = g_value_dup_object(value);
>          break;
>      case PROP_MONITOR:
>          priv->monitor = g_value_get_int(value);
>
> Then, I've also hit some issues with spice_channel_recv_link_hdr() if the guest
> is destroyed while the input channel waits to receive the link messages. When this
> happens, SpiceChannel::session is set to NULL in
> spice_session_channel_destroy(). spice_channel_recv_link_hdr() will jump to error: and attempt
> g_object_set(c->session, "protocol", 1, NULL); causing a critical as c->session is now NULL.

Ok, that sounds easily solvable. Not a show stopper.

>
> I suspect these are not the only places which could be impacted by such issues :(
>

fud? :)