On Sun, Feb 18, 2018 at 06:58:11PM +0000, 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 <fziglio@xxxxxxxxxx>
> ---
> 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", &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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel