Re: [Qemu-devel] [Spice-devel] [PATCH] spice: prepare for upcoming spice-server change

["Lukáš Hrázký" <lhrazky@redhat.com>]

On Fri, 2018-10-12 at 13:45 +0200, Gerd Hoffmann wrote:
> Future spice-server versions will call the client_monitors_config
> callback with the monitors list filtered to only include the monitors
> of the given display channel (aka QXLInstance).  Luckily this is easily
> detectable at runtime, so we can prepare for that in advance and also
> make qemu compatible with both old and new spice-server versions.
> 
> While being at it also use the console index instead of head number as
> array index.  The later doesn't work correctly in case multiple display
> devices are present.
> 
> Cc: spice-devel@lists.freedesktop.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

FWIW LGTM :)

With a small note below.

Reviewed-by: Lukáš Hrázký <lhrazky@redhat.com>

> ---
>  ui/spice-display.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 2f8adb6b9f..52f8cb5ae1 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -674,10 +674,28 @@ static int interface_client_monitors_config(QXLInstance *sin,
>  
>      memset(&info, 0, sizeof(info));
>  
> -    head = qemu_console_get_head(ssd->dcl.con);
> -    if (mc->num_of_monitors > head) {
> -        info.width  = mc->monitors[head].width;
> -        info.height = mc->monitors[head].height;
> +    if (mc->num_of_monitors == 1) {
> +        /*
> +         * New spice-server version which filters the list of monitors
> +         * to only include those that belong to our display channel.
> +         *
> +         * single-head configuration (where filtering doesn't matter)
> +         * takes this code path too.
> +         */
> +        info.width  = mc->monitors[0].width;
> +        info.height = mc->monitors[0].height;
> +    } else {
> +        /*
> +         * Old spice-server which gives us all monitors, so we have to
> +         * figure ourself which entry we need.  Array index is the
> +         * channel_id, which is the qemu console index, see
> +         * qemu_spice_add_display_interface().
> +         */
> +        head = qemu_console_get_index(ssd->dcl.con);
> +        if (mc->num_of_monitors > head) {

I consider it a convention to put the variable index (head) on the left
of the condition and the fixed size (mc->num_of_monitors) on the right.
So maybe swap those while touching the code.

Cheers,
Lukas

> +            info.width  = mc->monitors[head].width;
> +            info.height = mc->monitors[head].height;
> +        }
>      }
>  
>      trace_qemu_spice_ui_info(ssd->qxl.id, info.width, info.height);