[virt-tools-list] [PATCH virt-viewer v2] Fix a regression when initial connection fails
Jonathon Jongsma
jjongsma at redhat.com
Tue Apr 16 20:27:27 UTC 2019
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 at redhat.com> wrote:
> >
> > Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> >
> > 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 at redhat.com>
>
> 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 at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
>
More information about the virt-tools-list
mailing list