Re: [Spice-devel] [PATCH spice-server 5/8] stream-device: Workaround Qemu bug closing device
On Sun, Feb 18, 2018 at 06:58:11PM +, Frediano Ziglio wrote:
> Previous patch causes a bug into Qemu if the patch
"in Qemu" rather than "into"
> 46764fe09ca2e0f15c0981a672c166ed8cf57e72 ("virtio-serial: fix segfault
> on disconnect") is not include in that version of Qemu.
"included". I'd add a mention that this patch is present in Qemu 2.10.0
and newer.
> This crash happens when device is closed inside a write operation.
"when the device is closed during a write operation"
> For SPICE character device a spice_server_char_device_wakeup is called
"For SPICE character devices, 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
"we can't do it inside the handler without causing a crash, this commit
schedules a timer ..."
> 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.
"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
> ---
> 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 688a4e07..170c8637 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"
Add a dot at the end of this line
> +// as calling sif->state here can cause a crash schedule
"As calling sif->state here can cause a crash, schedule a timer and do
the call in it"
> +// a timer and do the call in it. Remove this code when
> +// will be sure all Qemu versions has been patched.
"when we are sure all Qemu versions have been patched"
(this will never happen, though when the 2 latest releases of main
distros don't have the bug in their QEMU, then I'd say we can remove
this).
> +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);
NB: I have some bad memories of g_timeout_add() not doing the right
thing at all when called with a timeout of 0ms. We'd need g_idle_add()
instead here, but that's not something which our interface allows
(this is just a comment in passing, not something we can act on right
now).
This makes me wonder if this could be racy though, if we get some writes
to the device before this delayed callback runs?
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 5/8] stream-device: Workaround Qemu bug closing device
On Sun, Feb 18, 2018 at 06:58:11PM +, Frediano Ziglio wrote:
> 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
> ---
> 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 688a4e07..170c8637 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
> +// 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;
> }
>
> @@ -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));
> @@ -562,7 +588,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", , NULL);
> @@ -570,7 +596,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);
> }
> }
>
> @@ -599,7 +625,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);
I would squash this part of the change in the previous commit.
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 5/8] stream-device: Workaround Qemu bug closing device
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
---
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 688a4e07..170c8637 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
+// 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;
}
@@ -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));
@@ -562,7 +588,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", , NULL);
@@ -570,7 +596,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);
}
}
@@ -599,7 +625,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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
