[Spice-commits] 4 commits - server/stream-device.c server/tests

Frediano Ziglio fziglio at kemper.freedesktop.org
Tue Mar 6 12:58:27 UTC 2018


 server/stream-device.c            |   46 +++++++++++++++++++++++++++++++++++++-
 server/tests/test-stream-device.c |   42 ++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 1 deletion(-)

New commits:
commit e45e087617279a8b3cba389743c14f65717ddbed
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Jan 23 13:00:39 2018 +0000

    test-stream-device: Check we don't read past data message
    
    Test case for the issue fixed by previous commit.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-stream-device.c b/server/tests/test-stream-device.c
index 7c573acc..3c9209a4 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -286,6 +286,47 @@ static void test_stream_device_multiple(void)
     basic_event_loop_destroy();
 }
 
+// check if data message consume even following message
+static void test_stream_device_format_after_data(void)
+{
+    uint8_t *p = message;
+    SpiceCoreInterface *core = basic_event_loop_init();
+    Test *test = test_new(core);
+
+    pos = 0;
+    vmc_write_pos = 0;
+    message_sizes_curr = message_sizes;
+    message_sizes_end = message_sizes;
+
+    // add some messages into device buffer
+    p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+    p = add_stream_hdr(p, STREAM_TYPE_DATA, 5);
+    memcpy(p, "hello", 5);
+    p += 5;
+    p = add_stream_hdr(p, STREAM_TYPE_INVALID, 0);
+    *message_sizes_end = p - message;
+    ++message_sizes_end;
+
+    vmc_instance.base.sif = &vmc_interface.base;
+    spice_server_add_interface(test->server, &vmc_instance.base);
+
+    // we need to open the device and kick the start
+    // the alarm is to avoid program to stuck
+    alarm(5);
+    spice_server_port_event(&vmc_instance, SPICE_PORT_EVENT_OPENED);
+    spice_server_char_device_wakeup(&vmc_instance);
+    alarm(0);
+
+    // we should read all data
+    g_assert(message_sizes_curr - message_sizes == 1);
+
+    // we should have an error back
+    check_vmc_error_message();
+
+    test_destroy(test);
+    basic_event_loop_destroy();
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
@@ -293,6 +334,7 @@ int main(int argc, char *argv[])
     g_test_add_func("/server/stream-device", test_stream_device);
     g_test_add_func("/server/stream-device-unfinished", test_stream_device_unfinished);
     g_test_add_func("/server/stream-device-multiple", test_stream_device_multiple);
+    g_test_add_func("/server/stream-device-format-after-data", test_stream_device_format_after_data);
 
     return g_test_run();
 }
commit ac57ee547a38497a1e4d25b2123d36940f89a0a2
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Jan 23 10:23:01 2018 +0000

    stream-device: Do not read past data message
    
    If data message is followed by another message, it's theoretically
    possible that device loses the sync with the guest.
    The actual Qemu and streaming agent implementation avoids it, but better to
    make sure this can't happen in the server code too.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/stream-device.c b/server/stream-device.c
index 86204663..dc58dce6 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -281,7 +281,7 @@ handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 
     while (1) {
         uint8_t buf[16 * 1024];
-        n = sif->read(sin, buf, sizeof(buf));
+        n = sif->read(sin, buf, MIN(sizeof(buf), dev->hdr.size));
         if (n <= 0) {
             break;
         }
commit a88533a21b3e343ff517451f89559418c9c43368
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Feb 27 11:48:26 2018 +0000

    stream-device: Workaround Qemu bug closing device
    
    Previous patch causes a bug in Qemu if the patch
    46764fe09ca2e0f15c0981a672c166ed8cf57e72 ("virtio-serial: fix segfault
    on disconnect") is not included in that version of Qemu (patch present in
    version 2.10.0).
    This crash happens when device is closed during a write operation.
    For SPICE character device, 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
    without causing a crash, this commit schedules a timer that will close the
    guest device outside this callback.
    The proper solution would be to patch Qemu but making sure of this is not
    so easy, hence this workaround in spice-server.
    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>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/stream-device.c b/server/stream-device.c
index e4df9940..86204663 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -57,6 +57,7 @@ struct StreamDevice {
     bool flow_stopped;
     StreamChannel *stream_channel;
     CursorChannel *cursor_channel;
+    SpiceTimer *close_timer;
 };
 
 struct StreamDeviceClass {
@@ -68,6 +69,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;
 
@@ -77,6 +80,16 @@ static StreamMsgHandler handle_msg_format, handle_msg_data, handle_msg_cursor_se
 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)
 {
@@ -94,7 +107,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
+        // we are sure all Qemu versions have 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;
     }
 
@@ -501,6 +524,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));
commit 52a36fabde09c9b7aa1cbdc042f8f710f64657bc
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Feb 13 15:16:35 2018 +0000

    stream-device: Disable guest device on errors
    
    Once the device is an error state, we don't want the guest to keep
    reading/writing to it, especially as this could put the device in an
    inconsistent state. This commit disables the device when an error occurs to
    prevent further unintended use of the device by the guest.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/stream-device.c b/server/stream-device.c
index ca28800c..e4df9940 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -94,6 +94,7 @@ stream_device_partial_read(StreamDevice *dev, SpiceCharDeviceInstance *sin)
         while (sif->read(sin, buf, sizeof(buf)) > 0) {
             continue;
         }
+        sif->state(sin, 0);
         return false;
     }
 
@@ -561,6 +562,19 @@ reset_channels(StreamDevice *dev)
 }
 
 static void
+char_device_set_state(RedCharDevice *char_dev, int state)
+{
+    SpiceCharDeviceInstance *sin = NULL;
+    g_object_get(char_dev, "sin", &sin, NULL);
+    spice_assert(sin != NULL);
+
+    SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
+    if (sif->state) {
+        sif->state(sin, state);
+    }
+}
+
+static void
 stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
 {
     if (event != SPICE_PORT_EVENT_OPENED && event != SPICE_PORT_EVENT_CLOSED) {
@@ -580,6 +594,10 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
     dev->flow_stopped = false;
     red_char_device_reset(char_dev);
     reset_channels(dev);
+
+    // enable the device again. We re-enable it on close as otherwise we don't want to get a
+    // failure when  we try to re-open the device as would happen if we keep it disabled
+    char_device_set_state(char_dev, 1);
 }
 
 static void


More information about the Spice-commits mailing list