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