[Spice-devel] [PATCH spice-server v2 2/2] stream-device: Implements properly device reset on open/close
Frediano Ziglio
fziglio at redhat.com
Thu Dec 7 08:47:39 UTC 2017
Due to the way Qemu handle the device we must consume all pending
data inside the callback to read data from the device.
We need to consume all data from previous device dialog to avoid
that next device usage is still seeing old data.
This as Qemu return 0 if you call SpiceCharDeviceInterface::read
outside this callback (which is called by Qemu using
spice_server_char_device_wakeup).
Also to communicate the error and avoiding to have to read any data the
guest want to write disable the device.
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(-)
Patch is much smaller if viewed with space changes disabled.
Changes since v1:
- disable device state;
- added back check for error from device.
diff --git a/server/stream-device.c b/server/stream-device.c
index 897fc665..735f2933 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 85c7ab1a..39b7382c 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();
@@ -189,6 +221,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;
@@ -210,6 +243,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