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

Gerd Hoffmann posted 1 patch 7 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181012114551.28809-1-kraxel@redhat.com
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
ui/spice-display.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
[Qemu-devel] [PATCH] spice: prepare for upcoming spice-server change
Posted by Gerd Hoffmann 7 years, 3 months ago
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>
---
 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) {
+            info.width  = mc->monitors[head].width;
+            info.height = mc->monitors[head].height;
+        }
     }
 
     trace_qemu_spice_ui_info(ssd->qxl.id, info.width, info.height);
-- 
2.9.3


Re: [Qemu-devel] [Spice-devel] [PATCH] spice: prepare for upcoming spice-server change
Posted by Lukáš Hrázký 7 years, 3 months ago
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);