On Wed, 2019-04-10 at 18:19 +0200, Marc-André Lureau wrote: > Hi > > > On Fri, Feb 1, 2019 at 10:30 AM Christophe Fergeau < > cfergeau@xxxxxxxxxx> wrote: > > > > Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx> > > > > On Thu, Jan 31, 2019 at 09:04:11AM -0600, Jonathon Jongsma wrote: > > > Due to changes in commit 65ef66e4, when the initial connection > > > fails, > > > virt-viewer just sat quietly and didn't indicate what was wrong. > > > It also > > > did not exit as it did before. This is because we were using > > > virt_viewer_session_spice_channel_destroy() incorrectly. This > > > function > > > was intended to be a callback that is called to clean up the VV > > > session > > > when the SpiceSession tells us that a channel has been destroyed. > > > It > > > does not actually destroy the channel, it only cleans up > > > references to > > > that channel within virt-viewer. After calling this function, the > > > channel is not affected in any way. If the channel object was > > > valid > > > before calling the function, it will be valid and unchanged after > > > calling the function as well. > > > > > > The problem is that before commit 65ef66e4, this function > > > (_channel_destroy()) also had a side-effect of emitting a signal > > > that > > > made us think that the SpiceSession was disconnected when it was > > > not. > > > The application responded to this signal by exiting even though > > > the > > > session was not properly disconnected and cleaned up. > > > > > > We now no longer exit the application until the SpiceSession is > > > properly > > > disconnected and cleaned up. So we need to make sure that this > > > happens > > > when our initial connection fails. Therefore, when the main > > > channel > > > receives an error channel-event, we should not call > > > virt_viewer_session_spice_channel_destroy(). This function should > > > only > > > be called when a channel has actually been destroyed, and the > > > channel is > > > not destroyed at this point. We should instead explicitly > > > disconnect > > > the session, which will result in the channels being destroyed > > > properly. > > > After the session destroys all of the channels, the 'channel- > > > destroy' signal > > > will be emitted by SpiceSession, so the _channel_destroy() > > > function will > > > eventually get called by the signal handler. > > > > > > To make the proper use of the function more obvious, I also > > > changed the > > > function name from _channel_destroy() to _channel_destroyed() and > > > added > > > a comment. > > > > > > Fixes: rhbz#1666869 > > > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > > Why is this not applied? > > thanks Oversight apparently. Pushed now. Jonathon > > > > --- > > > > > > Changes in v2: > > > - change function name to _channel_destroyed() > > > > > > src/virt-viewer-session-spice.c | 19 ++++++++++--------- > > > 1 file changed, 10 insertions(+), 9 deletions(-) > > > > > > diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer- > > > session-spice.c > > > index f76c7f9..5e214cd 100644 > > > --- a/src/virt-viewer-session-spice.c > > > +++ b/src/virt-viewer-session-spice.c > > > @@ -72,9 +72,9 @@ static void > > > virt_viewer_session_spice_usb_device_selection(VirtViewerSession > > > *se > > > static void virt_viewer_session_spice_channel_new(SpiceSession > > > *s, > > > SpiceChannel > > > *channel, > > > VirtViewerSess > > > ion *session); > > > -static void > > > virt_viewer_session_spice_channel_destroy(SpiceSession *s, > > > - SpiceChann > > > el *channel, > > > - VirtViewer > > > Session *session); > > > +static void > > > virt_viewer_session_spice_channel_destroyed(SpiceSession *s, > > > + SpiceCha > > > nnel *channel, > > > + VirtView > > > erSession *session); > > > static void > > > virt_viewer_session_spice_session_disconnected(SpiceSession *s, > > > VirtV > > > iewerSessionSpice *session); > > > static void > > > virt_viewer_session_spice_smartcard_insert(VirtViewerSession > > > *session); > > > @@ -400,7 +400,7 @@ create_spice_session(VirtViewerSessionSpice > > > *self) > > > virt_viewer_signal_connect_object(self->priv->session, > > > "channel-new", > > > 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); > > > + G_CALLBACK(virt_viewer_ses > > > sion_spice_channel_destroyed), self, 0); > > > virt_viewer_signal_connect_object(self->priv->session, > > > "disconnected", > > > G_CALLBACK(virt_viewer_ses > > > sion_spice_session_disconnected), self, 0); > > > > > > @@ -776,14 +776,14 @@ > > > virt_viewer_session_spice_main_channel_event(SpiceChannel > > > *channel, > > > spice_session_connect(self->priv->session); > > > } > > > } else { > > > - virt_viewer_session_spice_channel_destroy(NULL, > > > channel, session); > > > + spice_session_disconnect(self->priv->session); > > > } > > > break; > > > } > > > case SPICE_CHANNEL_ERROR_IO: > > > case SPICE_CHANNEL_ERROR_LINK: > > > case SPICE_CHANNEL_ERROR_TLS: > > > - virt_viewer_session_spice_channel_destroy(NULL, channel, > > > session); > > > + spice_session_disconnect(self->priv->session); > > > break; > > > default: > > > g_warning("unhandled spice main channel event: %d", > > > event); > > > @@ -1111,10 +1111,11 @@ > > > virt_viewer_session_spice_session_disconnected(G_GNUC_UNUSED > > > SpiceSession *s, > > > g_signal_emit_by_name(self, "session-disconnected", error ? > > > error->message : NULL); > > > } > > > > > > +/* called when the spice session indicates that a session has > > > been destroyed */ > > > static void > > > -virt_viewer_session_spice_channel_destroy(G_GNUC_UNUSED > > > SpiceSession *s, > > > - SpiceChannel *channel, > > > - VirtViewerSession > > > *session) > > > +virt_viewer_session_spice_channel_destroyed(G_GNUC_UNUSED > > > SpiceSession *s, > > > + SpiceChannel > > > *channel, > > > + VirtViewerSession > > > *session) > > > { > > > VirtViewerSessionSpice *self = > > > VIRT_VIEWER_SESSION_SPICE(session); > > > int id; > > > -- > > > 2.17.2 > > > > > _______________________________________________ > > virt-tools-list mailing list > > virt-tools-list@xxxxxxxxxx > > https://www.redhat.com/mailman/listinfo/virt-tools-list > > _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list