[Spice-devel] [PATCH spice 2/3] Call read_from_vdi_port() from vdi_read_buf_release()

[Hans de Goede]

read_from_vdi_port(), called from vdagent_char_device_wakeup() may
fail to consume all data because no buffers are available in the
read_bufs ring. When this happens we would fail to ever read more data
from the agent on the guest as the port is throttled and stays throttled
until we've consumed all data from the current buffer.

This patch re-enables the call to read_from_vdi_port() from
vdi_read_buf_release(), so that we will try the read again when space
becomes available in the read_bufs ring.

Together with another nasty hack in the linux guest virtio_console
driver, where it waits for a write to be acked by the host before
continuing with the next one, this can lead to a linux guest
getting stuck / hang (until the write is read by the spice-server
which never happens becaus of the above issues).

Note that even with this patch, the guest will still gets stuck due to
a bug in watch_update_mask in spice-core in qemu, which causes writing
to the client to never resume once it blocked. A patch for this has been
submitted to qemu.
---
 server/reds.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 8e63062..5160b01 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1202,7 +1202,12 @@ void vdi_read_buf_release(uint8_t *data, void *opaque)
     VDIReadBuf *buf = (VDIReadBuf *)opaque;
 
     ring_add(&reds->agent_state.read_bufs, &buf->link);
-    //read_from_vdi_port(); // XXX WTF? - ask arnon. should we be calling this here?? (causes recursion of read_from_vdi_port..) 
+    /* read_from_vdi_port() may have never completed because the read_bufs
+       ring was empty. So we call it again so it can complete its work if
+       necessary. Note since we can be called from read_from_vdi_port ourselves
+       this can cause recursion, read_from_vdi_port() contains code protecting
+       it against this. */
+    while (read_from_vdi_port());
 }
 
 static void dispatch_vdi_port_data(int port, VDIReadBuf *buf)
@@ -1235,7 +1240,12 @@ static void dispatch_vdi_port_data(int port, VDIReadBuf *buf)
    it returns 0 ensures that all data has been consumed. */
 static int read_from_vdi_port(void)
 {
-    // FIXME: UGLY HACK. Result of spice-vmc vmc_read triggering flush of throttled data, and recalling this.
+    /* There are 2 scenarios where we can get called recursively:
+       1) spice-vmc vmc_read triggering flush of throttled data, recalling us
+       2) the buf we push to the client may be send immediately without
+          blocking, in which case its free function will recall us
+       This messes up the state machine, so ignore recursive calls.
+       This is why we always must be called in a loop. */
     static int inside_call = 0;
     int quit_loop = 0;
     VDIPortState *state = &reds->agent_state;
-- 
1.7.3.1