[Spice-devel] [PATCH spice-server v3 3/3] stream-device: Implements properly device reset on open/close
Frediano Ziglio
fziglio at redhat.com
Thu Jan 11 12:06:11 UTC 2018
Due to the way Qemu handle character devices we must consume all pending
data inside the callback to read data from the device (in this case
stream_device_read_msg_from_dev).
We need to consume all data from previous device communication to avoid
that next device usage is still seeing old data.
This needs to be done within this callback, as QEMU returns 0 if you
call SpiceCharDeviceInterface::read() outside of it. QEMU invokes this
callback through a call to spice_server_char_device_wakeup.
To disable the guest to continue writing to device if we got a protocol
error we disable the device calling SpiceCharDeviceInterface::state.
On the test now we must test that we receive an error from the device.
This as previously we checked that last part of the data was not read
but now potentially all data are readed so we need another way to check
the device detected the error.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
server/stream-device.c | 35 +++++++++++-
server/tests/test-stream-device.c | 108 +++++++++++++++++++++++++-------------
2 files changed, 105 insertions(+), 38 deletions(-)
diff --git a/server/stream-device.c b/server/stream-device.c
index 897fc665f..735f29339 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -79,11 +79,23 @@ stream_device_read_msg_from_dev(RedCharDevice *self, SpiceCharDeviceInstance *si
int n;
bool handled = false;
- if (dev->has_error || dev->flow_stopped || !dev->stream_channel) {
+ sif = spice_char_device_get_interface(sin);
+
+ // in order to get in sync every time we open the device we need
+ // to discard data here. This as Qemu keeps a buffer of data which
+ // is used only during spice_server_char_device_wakeup from Qemu
+ if (G_UNLIKELY(dev->has_error)) {
+ uint8_t buf[16 * 1024];
+ while (sif->read(sin, buf, sizeof(buf)) > 0) {
+ continue;
+ }
+ sif->state(sin, 0);
return NULL;
}
- sif = spice_char_device_get_interface(sin);
+ if (dev->flow_stopped || !dev->stream_channel) {
+ return NULL;
+ }
/* read header */
while (dev->hdr_pos < sizeof(dev->hdr)) {
@@ -353,6 +365,19 @@ reset_channels(StreamDevice *dev)
}
}
+static void
+char_device_enable(RedCharDevice *char_dev)
+{
+ 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, 1);
+ }
+}
+
static void
stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
{
@@ -373,6 +398,12 @@ 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 try to enable it even on close as
+ // 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);
}
static void
diff --git a/server/tests/test-stream-device.c b/server/tests/test-stream-device.c
index 0e0c4974e..26c213105 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -23,6 +23,7 @@
#include <string.h>
#include <stdlib.h>
#include <stdio.h>
+#include <stdbool.h>
#include <unistd.h>
#include <spice/protocol.h>
@@ -42,13 +43,20 @@ static unsigned pos;
// then the size is reach we move on next one till exausted
static unsigned message_sizes[16];
static unsigned *message_sizes_end, *message_sizes_curr;
+static bool device_enabled = false;
+
+static unsigned vmc_write_pos;
+static uint8_t vmc_write_buf[2048];
// handle writes to the device
static int vmc_write(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
SPICE_GNUC_UNUSED const uint8_t *buf,
int len)
{
- // currently we don't test anything on write so reply we wrote it
+ // just copy into the buffer
+ unsigned copy = MIN(sizeof(vmc_write_buf) - vmc_write_pos, len);
+ memcpy(vmc_write_buf+vmc_write_pos, buf, copy);
+ vmc_write_pos += copy;
return len;
}
@@ -81,6 +89,7 @@ static int vmc_read(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
static void vmc_state(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
SPICE_GNUC_UNUSED int connected)
{
+ device_enabled = !!connected;
}
static SpiceCharDeviceInterface vmc_interface = {
@@ -126,57 +135,80 @@ static uint8_t *add_format(uint8_t *p, uint32_t w, uint32_t h, SpiceVideoCodecTy
return p + sizeof(fmt);
}
+// check we have an error message on the write buffer
+static void
+check_vmc_error_message(void)
+{
+ StreamDevHeader hdr;
+
+ g_assert(vmc_write_pos >= sizeof(hdr));
+
+ memcpy(&hdr, vmc_write_buf, sizeof(hdr));
+ g_assert_cmpint(hdr.protocol_version, ==, STREAM_DEVICE_PROTOCOL);
+ g_assert_cmpint(GUINT16_FROM_LE(hdr.type), ==, STREAM_TYPE_NOTIFY_ERROR);
+ g_assert_cmpint(GUINT32_FROM_LE(hdr.size), <=, vmc_write_pos - sizeof(hdr));
+}
+
static void test_stream_device(void)
{
uint8_t *p = message;
SpiceCoreInterface *core = basic_event_loop_init();
Test *test = test_new(core);
- message_sizes_curr = message_sizes;
- message_sizes_end = message_sizes;
+ for (int test_num=0; test_num < 2; ++test_num) {
+ pos = 0;
+ vmc_write_pos = 0;
+ message_sizes_curr = message_sizes;
+ message_sizes_end = message_sizes;
- // add some messages into device buffer
- // here we are testing the device is reading at least two
- // consecutive format messages
- // first message part has 2 extra bytes to check for header split
- p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
- *message_sizes_end = p - message + 2;
- ++message_sizes_end;
+ // add some messages into device buffer
+ // here we are testing the device is reading at least two
+ // consecutive format messages
+ // first message part has 2 extra bytes to check for header split
+ p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+ *message_sizes_end = p - message + 2;
+ ++message_sizes_end;
- p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_VP9);
+ p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_VP9);
- // this split the second format in half
- *message_sizes_end = p - message - 4;
- ++message_sizes_end;
+ // this split the second format in half
+ *message_sizes_end = p - message - 4;
+ ++message_sizes_end;
- *message_sizes_end = p - message;
- ++message_sizes_end;
+ *message_sizes_end = p - message;
+ ++message_sizes_end;
- // add a message to stop data to be read
- p = add_stream_hdr(p, STREAM_TYPE_INVALID, 0);
- *message_sizes_end = p - message;
- ++message_sizes_end;
+ // add a message to stop data to be read
+ p = add_stream_hdr(p, STREAM_TYPE_INVALID, 0);
+ *message_sizes_end = p - message;
+ ++message_sizes_end;
- // this message should not be read
- p = add_stream_hdr(p, STREAM_TYPE_INVALID, 0);
- *message_sizes_end = p - message;
- ++message_sizes_end;
+ // this message should not be read
+ 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);
+ vmc_instance.base.sif = &vmc_interface.base;
+ spice_server_add_interface(test->server, &vmc_instance.base);
- // device should not have read data before we open it
- spice_server_char_device_wakeup(&vmc_instance);
- g_assert_cmpint(pos, ==, 0);
+ // device should not have read data before we open it
+ spice_server_char_device_wakeup(&vmc_instance);
+ g_assert_cmpint(pos, ==, 0);
- // we need to open the device and kick the start
- spice_server_port_event(&vmc_instance, SPICE_PORT_EVENT_OPENED);
- spice_server_char_device_wakeup(&vmc_instance);
+ // we need to open the device and kick the start
+ spice_server_port_event(&vmc_instance, SPICE_PORT_EVENT_OPENED);
+ spice_server_char_device_wakeup(&vmc_instance);
+ spice_server_port_event(&vmc_instance, SPICE_PORT_EVENT_CLOSED);
- // make sure first 3 parts are read completely
- g_assert(message_sizes_curr - message_sizes >= 3);
- // make sure last part is not read at all
- g_assert(message_sizes_curr - message_sizes < 5);
+ // make sure first 3 parts are read completely
+ g_assert(message_sizes_curr - message_sizes >= 3);
+ // make sure the device readed all or that device was
+ // disabled, we need this to make sure that device will be in
+ // sync when opened again
+ g_assert(message_sizes_curr - message_sizes == 5 || !device_enabled);
+
+ check_vmc_error_message();
+ }
test_destroy(test);
basic_event_loop_destroy();
@@ -190,6 +222,7 @@ static void test_stream_device_unfinished(void)
Test *test = test_new(core);
pos = 0;
+ vmc_write_pos = 0;
message_sizes_curr = message_sizes;
message_sizes_end = message_sizes;
@@ -211,6 +244,9 @@ static void test_stream_device_unfinished(void)
// we should read all data
g_assert(message_sizes_curr - message_sizes == 1);
+ // we should have no data from the device
+ g_assert_cmpint(vmc_write_pos, ==, 0);
+
test_destroy(test);
basic_event_loop_destroy();
}
--
2.14.3
More information about the Spice-devel
mailing list