[Spice-devel] [PATCH v2] Don't close all but one display during reboot.

[Jonathon Jongsma]

On Thu, 2017-05-25 at 08:14 -0400, Frediano Ziglio wrote:
> > 
> > When a guest is rebooted, the QXL driver gets unloaded at some
> > point in
> > the reboot process. When the driver is unloaded, the spice server
> > sets a
> > single flag to FALSE: RedWorker::driver_cap_monitors_config. This
> > flag
> > indicates whether the driver is capable of sending its own monitors
> > config
> > messages to the client.
> > 
> > The only place this flag is used is when a new primary surface is
> > created. If
> > this flag is true, the server assumes that the driver will send its
> > own
> > monitors config very soon after the surface is created. If it's
> > false, the
> > server directly sends its own temporary monitors config message to
> > the client
> > based on the size of the just-created primary surface. This
> > temporary monitors
> > config message always has a maximum monitor count of 1.
> > 
> > This flag is set to false at startup so that in early boot (e.g.
> > vga text
> > mode), the server will send out these 'temporary' monitor config
> > messages to
> > the client whenever the primary surface is destroyed and re-
> > created. This
> > causes the client to resize its window to match the size of the
> > guest. When the
> > QXL driver is loaded and starts taking over the monitors config
> > responsibilities, we set this flag to true and the server stops
> > sending
> > monitors config messages out on its own.
> > 
> > If we reboot and set thsi flag to false, it will result in the
> > server sending a
> 
> typo: thsi -> this
> 
> > monitors config message to the client indicating that the guest now
> > supports a
> > maximum of 1 monitor. If the guest happens to have more than one
> > display window
> > open, it will destroy those extra windows because they exceed the
> > maximum
> > allowed number of monitors. This means that if you reboot a guest
> > with 4
> > monitors, after reboot it will only have 1 monitor.
> > 
> > To avoid this, we assume that if we had the ability to support
> > multiple
> > monitors at some point, that will return at some point. So when the
> > server
> > constructs its own monitors config message to send to the client
> > (when the
> > driver_cap_monitors_config flag is false), we send the previous
> > maximum monitor
> > count instead of always sending a maximum of 1.
> > 
> > Resolves: rhbz#1274447
> > ---
> > 
> > I'm simply re-sending this patch to see if anybody has additional
> > opinions on
> > this patch. It is clearly a workaround, and there was some
> > skepticism about the
> > approach when I initially proposed it. I did answer several
> > concerns voiced by
> > people but the discussion sort of petered out without a conclusion.
> > I'm hoping
> > to get a firm ACK or NACK on the patch.
> > 
> 
> Maybe as workaround we should open a bug to remember to fix this.

The problem is, i'm not sure if there *is* a proper fix for this.
Perhaps you could change the client so that it doesn't close displays
that are currently opened when we receive a new monitors config message
with max-monitors < n_monitors. But that was an intentional choice in
the design of the client and changing it would potentially cause other
issues. I'm not sure it's worth changing.


> 
> Either way,
> 
> Acked-by: Frediano Ziglio <fziglio at redhat.com>
> 
> > 
> >  server/display-channel.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index bfff413..3864419 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2447,15 +2447,18 @@ void
> > display_channel_set_monitors_config_to_primary(DisplayChannel
> > *display)
> >  {
> >      DrawContext *context = &display->priv->surfaces[0].context;
> >      QXLHead head = { 0, };
> > +    uint16_t old_max = 1;
> >  
> >      spice_return_if_fail(display->priv-
> > >surfaces[0].context.canvas);
> >  
> > -    if (display->priv->monitors_config)
> > +    if (display->priv->monitors_config) {
> > +        old_max = display->priv->monitors_config->max_allowed;
> >          monitors_config_unref(display->priv->monitors_config);
> > +    }
> >  
> >      head.width = context->width;
> >      head.height = context->height;
> > -    display->priv->monitors_config = monitors_config_new(&head, 1,
> > 1);
> > +    display->priv->monitors_config = monitors_config_new(&head, 1,
> > old_max);
> >  }
> >  
> >  void display_channel_reset_image_cache(DisplayChannel *self)
> 
> Frediano