[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