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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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,
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
