[Spice-devel] [PATCH] Wait to send monitor config until agent caps are received

[Jonathon Jongsma]

On Thu, 2014-08-28 at 11:59 -0400, Marc-André Lureau wrote:
> 
> ----- Original Message -----
> > When the first display is disabled and the vdagent is restarted in the guest,
> > it sometimes becomes enabled. This appears to be caused by a race condition
> > when an agent becomes connected. When the agent becomes connected,
> > virt-viewer
> > triggers a display update (spice_main_send_monitor_config()). This display
> > update happens in a timeout handler, but the timeout interval is set to 0 (so
> > it behaves basically like an idle handler).
> > 
> > The race happens because spice_main_send_monitor_config() behaves slightly
> > differently depending on the agent's capabilities. And sometimes the idle
> > handler runs before the client and server have negotiated capabilities. In
> > this
> > case, we have to assume that the server does not support sparse monitor
> > configurations. So instead of sending down an update where display #0 is off
> > and display #1 is WxH, we send down an update that only a single display:
> > display #0 is WxH. This results in the first display becoming enabled.
> > 
> > To solve the issue, we trigger a display update when the agent becomes
> > connected, but give it a long timeout (e.g. 5 seconds). This gives some time
> 
> To avoid the timeout, we could rely on the agent caps reply to happen
> right after sending the client caps->request, no?

Perhaps I don't understand your suggestion, but this 5-second timeout is
only to handle the case where the server doesn't actually send an agent
caps message. In the normal case (where the server sends its
capabilities), there will be no timeout because we'll trigger a display
update immediately after receiving the agent caps reply...


> 
> > for the agent capabilities negotiation to occur. When we receive the agent
> > capabilities from the guest, we trigger an immediate display update
> > (cancelling
> > the previously-scheduled update). If we receive no agent capabilities
> > (perhaps
> > because we're connected to a very old server that doesn't send capabilities),
> > the display update will occur after the initial timeout.
> > 
> > Resolves: rhbz#1043782, rhbz#1032923
> > ---
> > 
> > NOTE: the 5-second timeout I've used here is rather arbitrary. I'm open to
> > modifying this value if somebody has a good argument for a shorter or longer
> > timeout.
> > 
> >  gtk/channel-main.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gtk/channel-main.c b/gtk/channel-main.c
> > index 7a299a4..cb873b7 100644
> > --- a/gtk/channel-main.c
> > +++ b/gtk/channel-main.c
> > @@ -1356,6 +1356,12 @@ static void agent_start(SpiceMainChannel *channel)
> >          agent_announce_caps(channel);
> >          agent_send_msg_queue(channel);
> >      }
> > +
> > +    // trigger a display update, but give it a long timeout to allow agent
> > +    // capabilities to be negotiated. When we receive the capabilities
> > message,
> > +    // the display update will happen immediately. This long timeout is to
> > +    // accomodate older servers that may not send capabilities.
> > +    update_display_timer(SPICE_MAIN_CHANNEL(channel), 5);
> >  }
> >  
> >  /* coroutine context  */
> > @@ -1508,7 +1514,6 @@ static void main_handle_mouse_mode(SpiceChannel
> > *channel, SpiceMsgIn *in)
> >  static void main_handle_agent_connected(SpiceChannel *channel, SpiceMsgIn
> >  *in)
> >  {
> >      agent_start(SPICE_MAIN_CHANNEL(channel));
> > -    update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
> >  }
> >  
> >  /* coroutine context */
> > @@ -1519,7 +1524,6 @@ static void
> > main_handle_agent_connected_tokens(SpiceChannel *channel, SpiceMsgIn
> >  
> >      c->agent_tokens = msg->num_tokens;
> >      agent_start(SPICE_MAIN_CHANNEL(channel));
> > -    update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
> >  }
> >  
> >  /* coroutine context */
> > @@ -1790,6 +1794,7 @@ static void main_agent_handle_msg(SpiceChannel
> > *channel,
> >          }
> >          c->agent_caps_received = true;
> >          g_coroutine_signal_emit(self, signals[SPICE_MAIN_AGENT_UPDATE], 0);
> > +        update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
> >  
> >          if (caps->request)
> >              agent_announce_caps(self);
> > --
> > 1.9.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >