[Spice-devel] [PATCH spice-gtk] Notify about existence of monitor for all display channels

[Pavel Grunt]

On Thu, 2015-07-30 at 17:05 -0400, Jonathon Jongsma wrote:
> 
> ----- Original Message -----
> > From: "Pavel Grunt" <pgrunt at redhat.com>
> > To: "Marc-André Lureau" <mlureau at redhat.com>
> > Cc: spice-devel at lists.freedesktop.org
> > Sent: Thursday, July 30, 2015 11:20:27 AM
> > Subject: Re: [Spice-devel] [PATCH spice-gtk] Notify about existence of 
> > monitor	for all display channels
> > 
> > Hi
> > 
> > On Thu, 2015-07-30 at 10:35 -0400, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > ----- Original Message -----
> > > > Windows guest can have disabled display on the display chanel #0, in
> > > > that case it will not be listed in virt-viewer's "View->Displays" menu
> > > > 
> > > > Resolves:
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=91489
> > > > ---
> > > > I think notify::monitors should stay in create_canvas(), because in that
> > > > function the monitor
> > > > configuration changes (if the channel does not have
> > > > SPICE_DISPLAY_CAP_MONITORS_CONFIG),
> > > > so the notify should be emitted. The notify is also emitted when
> > > > SPICE_MSG_DISPLAY_MONITORS_CONFIG
> > > > is recieved, Windows guests are getting this message only for enabled
> > > > displays.
> > > > ---
> > > >  src/channel-display.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/channel-display.c b/src/channel-display.c
> > > > index e3f03fc..8837bd0 100644
> > > > --- a/src/channel-display.c
> > > > +++ b/src/channel-display.c
> > > > @@ -811,8 +811,7 @@ static void spice_display_channel_up(SpiceChannel
> > > > *channel)
> > > >  
> > > >      /* if we are not using monitors config, notify of existence of
> > > >         this monitor */
> > > > -    if (channel->priv->channel_id != 0)
> > > > -        g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> > > > +    g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> > > >  
> > > 
> > > And for guests that do send MonitorConfig messages on channel 0, this is
> > > not
> > > going to race?
> > 
> > There should not be the race, this notify happens in _channel_up() and 
> > always
> > has monitors->len = 0, monitors_max  = 1. virt-viewer then creates
> > monitors_max
> > spice display widgets, but does not enable any of them because
> > monitors->len=0.
> > Later the SPICE_MSG_DISPLAY_MONITORS_CONFIG is received and the displays are
> > enabled in virt-viewer.
> 
> 
> In addition to what Pavel said, if there was a potential for a race on channel 
> 0, that possibility would also exist for channels 1, 2, and 3 (since they can 
> already be enabled when virt-viewer connects). So I don't see any problem with 
> treating all channels the same.
> 
> I'm comfortable ACKing this patch, but I'd like to have the comment above 
> this line of code changed or removed. It's a bit confusing.

Ok, I will change it to "notify of existence of this monitor"

Pavel
> 
> Jonathon