[Spice-devel] [PATCH spice-server v5 6/8] stream-device: Workaround Qemu bug closing device

[Frediano Ziglio]

Previous patch causes a bug into Qemu if the patch
46764fe09ca2e0f15c0981a672c166ed8cf57e72 ("virtio-serial: fix segfault
on disconnect") is not include in that version of Qemu.
This crash happens when device is closed inside a write operation.
For SPICE character device a spice_server_char_device_wakeup is called
to write data which handles both read and write pending operations.
As we want to close the device but we can't do it inside the handler
operation schedule a timer that will close the guest device outside
this callback.
The proper solution would be to patch Qemu but making sure this
is not so easy so this workaround patch.
Code is marked with some comments to remember to remove this
hack in a safe future.

Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
 server/stream-device.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/server/stream-device.c b/server/stream-device.c
index b7553c67..869d8841 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -52,6 +52,7 @@ struct StreamDevice {
     bool opened;
     bool flow_stopped;
     StreamChannel *stream_channel;
+    SpiceTimer *close_timer;
 };
 
 struct StreamDeviceClass {
@@ -63,6 +64,8 @@ static StreamDevice *stream_device_new(SpiceCharDeviceInstance *sin, RedsState *
 
 G_DEFINE_TYPE(StreamDevice, stream_device, RED_TYPE_CHAR_DEVICE)
 
+static void char_device_set_state(RedCharDevice *char_dev, int state);
+
 typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
     SPICE_GNUC_WARN_UNUSED_RESULT;
 
@@ -71,6 +74,16 @@ static StreamMsgHandler handle_msg_format, handle_msg_data;
 static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
                                const char *error_msg) SPICE_GNUC_WARN_UNUSED_RESULT;
 
+static void
+close_timer_func(void *opaque)
+{
+    StreamDevice *dev = opaque;
+
+    if (dev->opened && dev->has_error) {
+        char_device_set_state(RED_CHAR_DEVICE(dev), 0);
+    }
+}
+
 static bool
 stream_device_partial_read(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 {
@@ -88,7 +101,17 @@ stream_device_partial_read(StreamDevice *dev, SpiceCharDeviceInstance *sin)
         while (sif->read(sin, buf, sizeof(buf)) > 0) {
             continue;
         }
-        sif->state(sin, 0);
+
+        // This code is a workaround for a Qemu bug, see patch
+        // "stream-device: Workaround Qemu bug closing device"
+        // as calling sif->state here can cause a crash schedule
+        // a timer and do the call in it. Remove this code when
+        // will be sure all Qemu versions has been patched.
+        RedsState *reds = red_char_device_get_server(RED_CHAR_DEVICE(dev));
+        if (!dev->close_timer) {
+            dev->close_timer = reds_core_timer_add(reds, close_timer_func, dev);
+        }
+        reds_core_timer_start(reds, dev->close_timer, 0);
         return false;
     }
 
@@ -340,6 +363,9 @@ stream_device_dispose(GObject *object)
 {
     StreamDevice *dev = STREAM_DEVICE(object);
 
+    RedsState *reds = red_char_device_get_server(RED_CHAR_DEVICE(dev));
+    reds_core_timer_remove(reds, dev->close_timer);
+
     if (dev->stream_channel) {
         // close all current connections and drop the reference
         red_channel_destroy(RED_CHANNEL(dev->stream_channel));
@@ -376,7 +402,7 @@ reset_channels(StreamDevice *dev)
 }
 
 static void
-char_device_enable(RedCharDevice *char_dev)
+char_device_set_state(RedCharDevice *char_dev, int state)
 {
     SpiceCharDeviceInstance *sin = NULL;
     g_object_get(char_dev, "sin", &sin, NULL);
@@ -384,7 +410,7 @@ char_device_enable(RedCharDevice *char_dev)
 
     SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
     if (sif->state) {
-        sif->state(sin, 1);
+        sif->state(sin, state);
     }
 }
 
@@ -413,7 +439,7 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
     // this could prevent a possible race condition where we open the
     // device from the guest and it take some time to enable causing
     // temporary writing issues.
-    char_device_enable(char_dev);
+    char_device_set_state(char_dev, 1);
 }
 
 static void
-- 
2.14.3