Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

On Fri, Oct 12, 2018 at 06:46:37AM -0400, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > > > When using qemu_console_get_head() it doesn't work correctly, it would
> > > > use the qxl card's data.  It would work if spice-server would filter the
> > > > list to only include the entries for the given display channel before
> > > > calling the ->client_monitors_config() callback.  But it doesn't, we get
> > > > the complete set.
> > > 
> > > That's why I said is a bug, IMHO it should.
> > 
> > Hmm, this should be easily detectable on qemu side I think.  If
> > num_of_monitors (for a non-qxl card) is > 1, then we have an old,
> > non-filtering spice-server.  If num_of_monitors == 1, then it is a new,
> > filtering spice-server.  Or a single-head channel configuration, in
> > which case it doesn't matter whenever it filters or not.
> > 
> > So this should be fixable without causing too much compatibility pain.
> > 
> > cheers,
> >   Gerd
> > 
> 
> Sorry, why fixing in Qemu is the bug is in spice-server?
> I really don't follow your reasoning.

I had a thinko in one of the previous messages.
qemu_console_get_head() is never correct, so qemu is buggy too.

It should be either qemu_console_get_index() (equals channel id), for
the non-filtering case, or just 0, for the filtering case, because every
virtio-gpu head has its own display channel.  Patch below.

> For me Qemu should not change at all and spice-server should
> do the filtering. Is this not fixing everything?

Well, fixed qemu being able to deal with all spice-server versions
(fixed and unifixed) is certainly useful to reduce the damage caused
by this incompatible change.

The qxl version of that callback can stay as-is, the spice-server side
fix should make sure the current code continues to work when we start
supporting new configurations (qxl and non-qxl devices mixed).

cheers,
  Gerd

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