[Spice-devel] [PATCH spice-gtk 1/2] channel-display: Make monitors array contain monitors in id order

[Hans de Goede]

Hi,

On 01/16/2013 03:07 PM, Marc-André Lureau wrote:
>
>
> ----- Mensaje original -----
>> Both the spice-widget code, as well as remote-viewer expect that
>> monitors[i]->id == i, but if ie only the qxl-0 and qxl-2 outputs are
>> enabled this is not true.
>>
>> I've chosen to fix this by making this assumption true. This does
>> mean
>
> Your explanation is misleading, we don't know what monitors[] you are talking about. There is no such "expectation" in the code, ie nothing bad will happen:

Please, please stop being so damn stubborn, and listen to what other
people have to say once in a while.

You already made the same point at the beginning of this thread, and
I already pointed out the 2 places where the monitors property of
the display channel is actually used, and how both of them expect
that monitors[i]->id == i, to quote from that mail:

"
spice-widget.c: update_monitor_area()

     c = &g_array_index(monitors, SpiceDisplayMonitorConfig, d->monitor_id);


virt-viewer-session-spice.c: virt_viewer_session_spice_display_monitors():

         display = g_ptr_array_index(displays, monitor->id);
"

As for not knowing what monitors I'm talking about:
1) The titel of the patch is "channel-display: Make monitors	array contain monitors in id order"
So how about the monitors property of the display channel ?

2) This is also very clear from the code changes itself.


 >> that the monitors array can now contain 0x0 sized (iow disabled)
 >> monitors,
 >> remote-viewer already checks for this.
 >
 > Yes, because it builds its own array.

Not true, it gets the monitors property directly from the display channel:

virt-viewer-session-spice.c: virt_viewer_session_spice_display_monitors():

     g_object_get(channel,
                  "monitors", &monitors,
                  "monitors-max", &monitors_max,
                  NULL);

And then it creates max_monitors VirtViewerDisplaySpice
objects, and then walks the monitors array enabling
any VirtViewerDisplaySpice where width != 0 && height != 0:

     for (i = 0; i < monitors->len; i++) {
         SpiceDisplayMonitorConfig *monitor = &g_array_index(monitors, SpiceDisp
         display = g_ptr_array_index(displays, monitor->id);
         g_return_if_fail(display != NULL);

         if (monitor->width == 0 || monitor->width == 0)
             continue;

         virt_viewer_display_set_enabled(VIRT_VIEWER_DISPLAY(display), TRUE);
         virt_viewer_display_set_desktop_size(VIRT_VIEWER_DISPLAY(display),
                                              monitor->width, monitor->height);
     }


See how it is addressing the monitors property of the display-channel
by id !! let me single out the line where this happens for you (again):

         display = g_ptr_array_index(displays, monitor->id);

 >> Thus this patch:
 >> 1) Makes the monitors[i]->id == i assumption be true
 >
 > No need for changing that.

/me bangs head on table.

There is a need to change that because the monitors property of the display
channel is a 1 on 1 copy of the monitors message on the wire, and for that
message monitors[i]->id == i is simply *not* true, yet it is assumed in
2 places in the code, as I've pointed out *twice* now!

 >
 >> 2) Makes spice-widget check for 0x0 sized monitors and treat them as
 >> disabled
 >
 > Nor that.

again:

spice-widget.c: update_monitor_area()

     c = &g_array_index(monitors, SpiceDisplayMonitorConfig, d->monitor_id);

See how the array is indexed by id, since it can contain only 2 monitors,
with for example id 0 and id 3, this will index it out of bounds.

If we fix this by inserting 0x0 sized monitors, then the checking for
such monitors becomes necessary!

Regards,

Hans