[Spice-devel] [PATCH 0/2] RFC: handle startup race for monitors config

[Jonathon Jongsma]

On Thu, 2015-04-09 at 14:06 -0500, Jonathon Jongsma wrote:
> On Thu, 2015-04-02 at 11:15 -0500, Jonathon Jongsma wrote:
> > On Thu, 2015-04-02 at 16:42 +0200, Marc-André Lureau wrote:
> > > 
> > > On Thu, Apr 2, 2015 at 4:36 PM, Jonathon Jongsma <jjongsma at redhat.com>
> > > wrote:
> > >         That would be a pretty drastic change in behavior. So, no, I
> > >         have not
> > >         considered that. It would also open up a large can of worms.
> > >         For
> > >         example, what would happen if you re-configured the displays
> > >         from within
> > >         the guest control panel. What would you do then?
> > >         
> > > 
> > > 
> > > I would eventually follow guest configuration, but I wouldn't send
> > > back a new configuration (no auto-resize).
> > > 
> > 
> > 
> > So, I did a quick test where I used an un-modified spice-server and the
> > following diff to virt-viewer:
> > 
> > ===================
> > 
> > diff --git a/src/virt-viewer-session.c b/src/virt-viewer-session.c
> > index 131a500..5f47d09 100644
> > --- a/src/virt-viewer-session.c
> > +++ b/src/virt-viewer-session.c
> > @@ -412,6 +412,13 @@ virt_viewer_session_on_monitor_geometry_changed(VirtViewerSession* self,
> >      if (!klass->apply_monitor_geometry)
> >          return;
> >  
> > +    /* don't send monitor resize if we're in auto-conf fullscreen mode...*/
> > +    if (virt_viewer_app_get_fullscreen(self->priv->app)) {
> > +        g_debug("%s: not sending new monitors config because app is in fullscreen mode", G_STRFUNC);
> > +        return;
> > +    }
> > +    g_debug("%s: sending", G_STRFUNC);
> > +
> >      /* find highest monitor ID so we can create the sparse array */
> >      for (l = self->priv->displays; l; l = l->next) {
> >          VirtViewerDisplay *d = VIRT_VIEWER_DISPLAY(l->data);
> > 
> > ====================
> > 
> > 
> > This is basically what you suggest. While we're in auto-conf mode, it never
> > sends down monitor config changes, but it does respond to display updates from
> > the server. When the user exits fullscreen mode, it resumes auto-resize and
> > sends new monitor config messages to the server. It improved the situation, but
> > didn't fix it. Before applying this patch, when I started virt-viewer in
> > --full-screen mode, the extra display was left enabled about 25-50% of the
> > time. After this patch, it happens only about 10% of the time.
> 
> So here's the current state of this investigation. Just to recap, this
> is (as far as I can tell) the source of the problem:
> 
> [guest has 2 displays enabled]
>       * main channel connects
>       * client sends monitor config (only 1 display enabled) to server
>       * server sends new monitor config to guest
>       * display channel connects
>       * display channel sends out monitor config message with old state
>         (2 displays)
>       * client receives monitor config and enables 2 display windows
>       * guest finishes display configuration
>       * display channel sends out monitor config message with new state
>         (1 display)
>       * second display on client becomes "unready" ("waiting for display
>         2...")
>       * Some future re-allocation / etc causes the client to send down
>         new monitor configuration, which re-enables the "unready" second
>         display (see rhbz#868970 for why this happens and why it's very
>         difficult to solve)
> 
> There are two basic approaches that can be taken here. The first is the
> one that I originally proposed: handle it on the server side. The
> rationale for this approach is pretty straightforward: the server
> *knows* that it is sending out an invalid monitor configuration at
> startup (since it knows that it has already received a new monitor
> configuration from the client), and we should not send out wrong
> information knowingly. The change that I originally proposed is not
> particularly elegant, I'll admit. It does use a timeout to delay sending
> out our initial monitor configuration to give the guest a chance to
> reconfigure itself before we send the configuration. Obviously, if the
> guest takes longer to reconfigure displays than the timeout, we'll still
> see this issue. And a timeout inherently adds more chances for race
> conditions, etc. But in my testing (with host and client on different
> machines on a local network), I've never yet seen a failure with this
> patch applied.
> 
> The second approach is to change the client so that it can handle the
> server sending us an old configuration followed immediately by a new
> configuration. However, this is not trivial, and everything I've tried
> so far to handle it has opened up new regressions in other areas that
> are hard to predict and require very extensive testing to even find.
> It's clear that the client sends out too many unnecessary monitor
> reconfiguration messages. But preventing the client from sending out
> these messages is risky; it's not always immediately clear which ones
> are unnecessary. One of Marc-Andre's proposed patches doing that
> introduced 2 new regressions (mentioned in other places in this thread).
> It also did not fully solve the bug, though it did reduce the frequency
> by half or so. But even if we could manage to prevent all unnecessary
> monitor configuration messages, we still have the issue of the extra
> display and what to do with it (again, see rhbz#868970 for a discussion
> on this issue). Marc-Andre had a promising suggestion that when we're in
> fullscreen mode, we should simply follow the display state of the server
> and never attempt to modify it from the client (after the very initial
> configuration message). He further suggested that we could simply close
> displays that were disabled by the server in this case to avoid that
> display getting re-enabled accidentally. Unfortunately, after
> prototyping this, I realized that displays in fullscreen mode will not
> be maintained through a reboot. For example, if you had virt-viewer
> fullscreen on two monitors and then rebooted the guest, one of the
> monitors would simply disappear during the reboot. So I don't really
> think that's a very workable solution (In fact, I think this is the very
> reason that we don't automatically close displays that the server
> indicates are disabled; see rhbz#868970).
> 
> So, that's where I am. The server-side solution basically works but
> isn't very pretty. And I can't figure out a client-side solution that
> will work for all scenarios. I'd really appreciate additional ideas or
> comments.
> 
> Jonathon


So, I've got another potential solution for this issue that's entirely
done on the client side. See this thread on virt-tools-list:
https://www.redhat.com/archives/virt-tools-list/2015-April/msg00128.html

Comments appreciated, but should probably be sent to virt-tools-list.

Jonathon