[virt-tools-list] [virt-viewer] spice: avoid crashing when using multi-seat

[Jonathon Jongsma]

On Wed, 2016-03-30 at 20:08 +0200, Fabiano Fidêncio wrote:
> Jonathon,
> 
> Firstly, sorry for a really long delay replying to this email, I was
> pretty sure that I have already answered that :-\

...and now it's my turn to apologize. Sorry it got lost in other stuff.

> 
> On Thu, Mar 17, 2016 at 5:23 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> > On Tue, 2016-03-15 at 16:51 +0100, Fabiano Fidêncio wrote:
> > 
> > > As multi-seat configuration is not supported by spice, the best we can
> > > do for now is avoid crashing on this setup.
> > 
> > Could you expand this commit log to make it more obvious how this change
> > relates
> > to "multi-seat"?
> 
> "As we receive a monitor config from each of the graphics device, we
> end up having a channel-id for each of the graphics card, which may
> case some issues when one of the cards is a multi-head device."
> 
> > Actually, I think that "multi-seat" is not really the correct
> > term here. What you really mean is "more than one multi-head graphics
> > device", I
> > think.
> 
> I'm not sure about this, Jonathon.
> If you have 2 QXL (multi-head) devices you won't see it happening (I
> didn't, at least).


Perhaps it's just a terminology issue. I don't see how any of the code changes
in this patch are related to "multi-seat". Reading the referenced bug report, I
now see that the bug was triggered by switching to a tty ("Ctrl+alt+F2"), so I
guess that is where the "multi-seat" terminology came from? But if you look
strictly at the code changes here, the crash happens because
 virt_viewer_display_spice_new() returns NULL and we don't handle it well. And
 virt_viewer_display_spice_new() returns NULL because we're violating the
following assumption 

    // We don't allow monitorid != 0 && channelid != 0

And this check is there to try to enforce the requirement that we only support
guest that have either

A) a single graphics device with multiple displays (monitorid=0, displayid={0,
1, 2, 3}), or

B) multiple graphics devices with a single display each (monitorid={0, 1, 2, 3},
displayid=0). 

>From looking at the code, it seems strange to me that it wouldn't happen if you
were using 2 QXL devices, but maybe depends on whether you try to enable
additional displays or not?

Anyway, this is a minor issue. The code looks fine, I just found the commit
message a little bit confusing.

> 
> > 
> > Also (this is unrelated to your change):
> > I wonder if we should change virt_viewer_display_spice_new() to not use
> > g_return_val_if_fail() for this situation. The fact that g_return_*if_fail()
> > functions can be disabled by setting G_DISABLE_CHECKS indicates that they
> > should
> > only be used for programming errors, whereas this is a situation caused by
> > data
> > received over the spice protocol. Maybe something like:
> > 
> > 
> > diff --git a/src/virt-viewer-display-spice.c b/src/virt-viewer-display
> > -spice.c
> > index 5114833..c657047 100644
> > --- a/src/virt-viewer-display-spice.c
> > +++ b/src/virt-viewer-display-spice.c
> > @@ -286,8 +286,10 @@ virt_viewer_display_spice_new(VirtViewerSessionSpice
> > *session,
> >      g_return_val_if_fail(SPICE_IS_DISPLAY_CHANNEL(channel), NULL);
> > 
> >      g_object_get(channel, "channel-id", &channelid, NULL);
> > -    // We don't allow monitorid != 0 && channelid != 0
> > -    g_return_val_if_fail(channelid == 0 || monitorid == 0, NULL);
> > +    if (channelid == 0 || monitorid == 0) {
> > +        g_warning("Unsupported graphics configuration. spice-gtk only
> > supports
> > multiple graphics channels if they are single-head");
> > +        return NULL;
> > +    }
> > 
> >      self = g_object_new(VIRT_VIEWER_TYPE_DISPLAY_SPICE,
> >                          "session", session,
> > 
> > 
> > Probably that comment could be worded better though...
> 
> I do agree. Would you mind if I squash this (or something similar) to
> the original patch?
> 

Yes, feel free to squash something like this in if you'd like.


> > 
> > Jonathon
> > 
> > 
> > > 
> > > Resolves: rhbz#1250820
> > > 
> > > Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> > > ---
> > >  src/virt-viewer-session-spice.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session
> > > -spice.c
> > > index dc0c1ff..a3014a8 100644
> > > --- a/src/virt-viewer-session-spice.c
> > > +++ b/src/virt-viewer-session-spice.c
> > > @@ -836,8 +836,11 @@ static void
> > >  destroy_display(gpointer data)
> > >  {
> > >      VirtViewerDisplay *display = VIRT_VIEWER_DISPLAY(data);
> > > -    VirtViewerSession *session =
> > > virt_viewer_display_get_session(display);
> > > +    VirtViewerSession *session;
> > > 
> > > +    g_return_if_fail (display != NULL);
> > > +
> > > +    session = virt_viewer_display_get_session(display);
> > >      g_debug("Destroying spice display %p", display);
> > >      virt_viewer_session_remove_display(session, display);
> > >      g_object_unref(display);
> > > @@ -886,6 +889,9 @@
> > > virt_viewer_session_spice_display_monitors(SpiceChannel
> > > *channel,
> > >          display = g_ptr_array_index(displays, i);
> > >          if (display == NULL) {
> > >              display = virt_viewer_display_spice_new(self, channel, i);
> > > +            if (display == NULL)
> > > +                continue;
> > > +
> > >              g_debug("creating spice display (#:%d)",
> > > 
> > >  virt_viewer_display_get_nth(VIRT_VIEWER_DISPLAY(display)));
> > >              g_ptr_array_index(displays, i) = g_object_ref_sink(display);
> 
> Best Regards,