Re: [PATCH virt-viewer v2] Fix a regression when initial connection fails

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> > ---
> >
> > 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,
> >                                                    VirtViewerSession *session);
> > -static void virt_viewer_session_spice_channel_destroy(SpiceSession *s,
> > -                                                      SpiceChannel *channel,
> > -                                                      VirtViewerSession *session);
> > +static void virt_viewer_session_spice_channel_destroyed(SpiceSession *s,
> > +                                                        SpiceChannel *channel,
> > +                                                        VirtViewerSession *session);
> >  static void virt_viewer_session_spice_session_disconnected(SpiceSession *s,
> >                                                             VirtViewerSessionSpice *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_session_spice_channel_new), self, 0);
> >      virt_viewer_signal_connect_object(self->priv->session, "channel-destroy",
> > -                                      G_CALLBACK(virt_viewer_session_spice_channel_destroy), self, 0);
> > +                                      G_CALLBACK(virt_viewer_session_spice_channel_destroyed), self, 0);
> >      virt_viewer_signal_connect_object(self->priv->session, "disconnected",
> >                                        G_CALLBACK(virt_viewer_session_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



-- 
Marc-André Lureau

_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list




[Index of Archives]     [Linux Virtualization]     [KVM Development]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]     [Video 4 Linux]

  Powered by Linux