[virt-tools-list] [PATCH virt-viewer] Spice: listen for new 'SpiceSession::disconnected' signal

[Jonathon Jongsma]

On Thu, 2019-01-17 at 00:06 +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jan 4, 2019 at 11:08 PM Marc-André Lureau
> <marcandre.lureau at gmail.com> wrote:
> > 
> > Hi
> > 
> > On Tue, Sep 18, 2018 at 8:25 PM Jonathon Jongsma <
> > jjongsma at redhat.com> wrote:
> > > 
> > > Previously we were emitting the VirtViewerSession::session-
> > > disconnected
> > > when we got the Spice::session::channel-destroy signal for the
> > > last
> > > channel. However, since the channels are still valid at this
> > > point, and
> > > because VirtViewerApp quits the application in response to the
> > > session-disconnected signal, that means that the channels were
> > > never
> > > being properly freed. This was particularly problematic for the
> > > usbredir
> > > channel, which must disconnect any connected USB devices as part
> > > of its
> > > destruction. By using the new SpiceSession::disconnected signal
> > > instead,
> > > we can ensure that all channels have been disconnected and
> > > properly
> > > destroyed before quitting the application.
> > > ---
> > > 
> > > NOTE: this is an older patch fell through the cracks. I added the
> > > SpiceSession::disconnected signal a while ago and was waiting for
> > > an
> > > official spice-gtk release before adding this patch to virt-
> > > viewer. The
> > > new spice-gtk release has been out for a while, but this patch
> > > was
> > > forgotten until now.
> > > 
> > >  src/virt-viewer-session-spice.c | 24 +++++++++++++++++++-----
> > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> > This patch introduces a regression, virt-viewer now hangs when the
> > initial connection fails.
> > Reverting the patch works again
> > 
> > Jonathon, can you take a look?
> 
> To not loose track of this regression, I opened
> https://bugzilla.redhat.com/show_bug.cgi?id=1666869
> 
> Daniel, Bugzilla is not that great to track upstream issues. Would
> pagure issues be better?

Sorry, I forgot to reply to this email. I've got some time now so I've
started looking into it.

Jonathon


> 
> > 
> > thanks
> > 
> > > 
> > > diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-
> > > session-spice.c
> > > index fdc7004..cb06af2 100644
> > > --- a/src/virt-viewer-session-spice.c
> > > +++ b/src/virt-viewer-session-spice.c
> > > @@ -50,7 +50,7 @@ struct _VirtViewerSessionSpicePrivate {
> > >      guint pass_try;
> > >      gboolean did_auto_conf;
> > >      VirtViewerFileTransferDialog *file_transfer_dialog;
> > > -
> > > +    GError *disconnect_error;
> > >  };
> > > 
> > >  #define VIRT_VIEWER_SESSION_SPICE_GET_PRIVATE(o)
> > > (G_TYPE_INSTANCE_GET_PRIVATE((o), VIRT_VIEWER_TYPE_SESSION_SPICE,
> > > VirtViewerSessionSpicePrivate))
> > > @@ -75,6 +75,8 @@ static void
> > > virt_viewer_session_spice_channel_new(SpiceSession *s,
> > >  static void
> > > virt_viewer_session_spice_channel_destroy(SpiceSession *s,
> > >                                                        SpiceChann
> > > el *channel,
> > >                                                        VirtViewer
> > > Session *session);
> > > +static void
> > > virt_viewer_session_spice_session_disconnected(SpiceSession *s,
> > > +                                                           VirtV
> > > iewerSessionSpice *session);
> > >  static void
> > > virt_viewer_session_spice_smartcard_insert(VirtViewerSession
> > > *session);
> > >  static void
> > > virt_viewer_session_spice_smartcard_remove(VirtViewerSession
> > > *session);
> > >  static gboolean
> > > virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionS
> > > pice *self);
> > > @@ -152,6 +154,7 @@ virt_viewer_session_spice_dispose(GObject
> > > *obj)
> > >          gtk_widget_destroy(GTK_WIDGET(spice->priv-
> > > >file_transfer_dialog));
> > >          spice->priv->file_transfer_dialog = NULL;
> > >      }
> > > +    g_clear_error(&spice->priv->disconnect_error);
> > > 
> > >      G_OBJECT_CLASS(virt_viewer_session_spice_parent_class)-
> > > >dispose(obj);
> > >  }
> > > @@ -398,6 +401,8 @@ create_spice_session(VirtViewerSessionSpice
> > > *self)
> > >                                        G_CALLBACK(virt_viewer_ses
> > > sion_spice_channel_new), self, 0);
> > >      virt_viewer_signal_connect_object(self->priv->session,
> > > "channel-destroy",
> > >                                        G_CALLBACK(virt_viewer_ses
> > > sion_spice_channel_destroy), self, 0);
> > > +    virt_viewer_signal_connect_object(self->priv->session,
> > > "disconnected",
> > > +                                      G_CALLBACK(virt_viewer_ses
> > > sion_spice_session_disconnected), self, 0);
> > > 
> > >      usb_manager = spice_usb_device_manager_get(self->priv-
> > > >session, NULL);
> > >      if (usb_manager) {
> > > @@ -1091,6 +1096,13 @@
> > > virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionS
> > > pice *self)
> > >      return TRUE;
> > >  }
> > > 
> > > +static void
> > > +virt_viewer_session_spice_session_disconnected(G_GNUC_UNUSED
> > > SpiceSession *s,
> > > +                                               VirtViewerSession
> > > Spice *self)
> > > +{
> > > +    g_signal_emit_by_name(self, "session-disconnected", self-
> > > >priv->disconnect_error);
> > > +}
> > > +
> > >  static void
> > >  virt_viewer_session_spice_channel_destroy(G_GNUC_UNUSED
> > > SpiceSession *s,
> > >                                            SpiceChannel *channel,
> > > @@ -1129,10 +1141,12 @@
> > > virt_viewer_session_spice_channel_destroy(G_GNUC_UNUSED
> > > SpiceSession *s,
> > >          if (self->priv->usbredir_channel_count == 0)
> > >              virt_viewer_session_set_has_usbredir(session,
> > > FALSE);
> > >      }
> > > -
> > > -    self->priv->channel_count--;
> > > -    if (self->priv->channel_count == 0)
> > > -        g_signal_emit_by_name(self, "session-disconnected",
> > > error ? error->message : NULL);
> > > +    if (error) {
> > > +        g_warning("Channel error: %s", error->message);
> > > +        if (self->priv->disconnect_error == NULL) {
> > > +            self->priv->disconnect_error = g_error_copy(error);
> > > +        }
> > > +    }
> > >  }
> > > 
> > >  VirtViewerSession *
> > > --
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > virt-tools-list mailing list
> > > virt-tools-list at redhat.com
> > > https://www.redhat.com/mailman/listinfo/virt-tools-list
> > 
> > 
> > 
> > --
> > Marc-André Lureau
> 
> 
>