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

[Fabiano Fidêncio]

Jonathon,

Firstly, sorry for a really long delay replying to this email, I was
pretty sure that I have already answered that :-\

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).

>
> 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?

>
> 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,