[Qemu-devel] [PATCH] hw/virtio-serial-bus: replay guest open on destination

[Qemu-devel] [PATCH] hw/virtio-serial-bus: replay guest open on destination

[Alon Levy]

When migrating a host with with a spice agent running the mouse becomes
non operational after the migration due to the agent state being
inconsistent between the guest and the client if the client is using
semi-seamless or switch host migration.

After migration the target client has never received the guest_open
initiated spice message. Virtio-serial holds this state information and
migrates it, so replay that over the chardev post migration. Fix is not
spice specific but spice is the only client that cares about it.

rhbz #725965.
---
 hw/virtio-serial-bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index efa8a81..ccce1fa 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -641,6 +641,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
     uint8_t host_connected;
+    VirtIOSerialPortClass *vsc;
 
     for (i = 0 ; i < s->post_load.nr_active_ports; ++i) {
         port = s->post_load.connected[i].port;
@@ -653,6 +654,11 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
             send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
                                port->host_connected);
         }
+        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+        if (port->guest_connected && vsc->guest_open) {
+            /* replay guest open */
+            vsc->guest_open(port);
+        }
     }
     g_free(s->post_load.connected);
     s->post_load.connected = NULL;
-- 
1.8.0

Re: [Qemu-devel] [PATCH] hw/virtio-serial-bus: replay guest open on destination

[Amit Shah]

On (Sun) 25 Nov 2012 [15:39:14], Alon Levy wrote:
> When migrating a host with with a spice agent running the mouse becomes
> non operational after the migration due to the agent state being
> inconsistent between the guest and the client if the client is using
> semi-seamless or switch host migration.
> 
> After migration the target client has never received the guest_open
> initiated spice message. Virtio-serial holds this state information and
> migrates it, so replay that over the chardev post migration. Fix is not
> spice specific but spice is the only client that cares about it.

Thanks for continuing to pursue this :)

> rhbz #725965.
> ---
>  hw/virtio-serial-bus.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index efa8a81..ccce1fa 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -641,6 +641,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>      VirtIOSerial *s = opaque;
>      VirtIOSerialPort *port;
>      uint8_t host_connected;
> +    VirtIOSerialPortClass *vsc;
>  
>      for (i = 0 ; i < s->post_load.nr_active_ports; ++i) {
>          port = s->post_load.connected[i].port;
> @@ -653,6 +654,11 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>              send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
>                                 port->host_connected);
>          }
> +        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> +        if (port->guest_connected && vsc->guest_open) {
> +            /* replay guest open */
> +            vsc->guest_open(port);
> +        }

I think the last time we discussed this, my objection was the guest
isn't really doing an open again, and since spice depends on the
guest's connectedness, spice should have a post-load hook or a similar
bh that would query virtio-serial for guest connectedness status, and,
if connected, do whatever setup is necessary.

Adding Gerd and Markus as I think they were involved in the discussion
last time as well.

		Amit

Re: [Qemu-devel] [PATCH] hw/virtio-serial-bus: replay guest open on destination

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Sun) 25 Nov 2012 [15:39:14], Alon Levy wrote:
>> When migrating a host with with a spice agent running the mouse becomes
>> non operational after the migration due to the agent state being
>> inconsistent between the guest and the client if the client is using
>> semi-seamless or switch host migration.
>> 
>> After migration the target client has never received the guest_open
>> initiated spice message. Virtio-serial holds this state information and
>> migrates it, so replay that over the chardev post migration. Fix is not
>> spice specific but spice is the only client that cares about it.
>
> Thanks for continuing to pursue this :)
>
>> rhbz #725965.
>> ---
>>  hw/virtio-serial-bus.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> index efa8a81..ccce1fa 100644
>> --- a/hw/virtio-serial-bus.c
>> +++ b/hw/virtio-serial-bus.c
>> @@ -641,6 +641,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>>      VirtIOSerial *s = opaque;
>>      VirtIOSerialPort *port;
>>      uint8_t host_connected;
>> +    VirtIOSerialPortClass *vsc;
>>  
>>      for (i = 0 ; i < s->post_load.nr_active_ports; ++i) {
>>          port = s->post_load.connected[i].port;
>> @@ -653,6 +654,11 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>>              send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
>>                                 port->host_connected);
>>          }
>> +        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>> +        if (port->guest_connected && vsc->guest_open) {
>> +            /* replay guest open */
>> +            vsc->guest_open(port);
>> +        }
>
> I think the last time we discussed this, my objection was the guest
> isn't really doing an open again, and since spice depends on the
> guest's connectedness, spice should have a post-load hook or a similar
> bh that would query virtio-serial for guest connectedness status, and,
> if connected, do whatever setup is necessary.
>
> Adding Gerd and Markus as I think they were involved in the discussion
> last time as well.

Got a pointer to the old thread?

Re: [Qemu-devel] [PATCH] hw/virtio-serial-bus: replay guest open on destination

[Amit Shah]

On (Tue) 27 Nov 2012 [15:10:06], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:

> > Adding Gerd and Markus as I think they were involved in the discussion
> > last time as well.
> 
> Got a pointer to the old thread?

https://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg02986.html

I didn't go through it entirely, but I'm looking for guidance here --
should spice handle the migration (as I suggest), or should
virtio-serial-bus replay guest_open (as this patch by Alon does)?

		Amit

Re: [Qemu-devel] [PATCH] hw/virtio-serial-bus: replay guest open on destination

[Anthony Liguori]

Amit Shah <amit.shah@redhat.com> writes:

> On (Sun) 25 Nov 2012 [15:39:14], Alon Levy wrote:
>> When migrating a host with with a spice agent running the mouse becomes
>> non operational after the migration due to the agent state being
>> inconsistent between the guest and the client if the client is using
>> semi-seamless or switch host migration.
>> 
>> After migration the target client has never received the guest_open
>> initiated spice message. Virtio-serial holds this state information and
>> migrates it, so replay that over the chardev post migration. Fix is not
>> spice specific but spice is the only client that cares about it.
>
> Thanks for continuing to pursue this :)
>
>> rhbz #725965.
>> ---
>>  hw/virtio-serial-bus.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> index efa8a81..ccce1fa 100644
>> --- a/hw/virtio-serial-bus.c
>> +++ b/hw/virtio-serial-bus.c
>> @@ -641,6 +641,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>>      VirtIOSerial *s = opaque;
>>      VirtIOSerialPort *port;
>>      uint8_t host_connected;
>> +    VirtIOSerialPortClass *vsc;
>>  
>>      for (i = 0 ; i < s->post_load.nr_active_ports; ++i) {
>>          port = s->post_load.connected[i].port;
>> @@ -653,6 +654,11 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>>              send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
>>                                 port->host_connected);
>>          }
>> +        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>> +        if (port->guest_connected && vsc->guest_open) {
>> +            /* replay guest open */
>> +            vsc->guest_open(port);
>> +        }
>
> I think the last time we discussed this, my objection was the guest
> isn't really doing an open again, and since spice depends on the
> guest's connectedness, spice should have a post-load hook or a similar
> bh that would query virtio-serial for guest connectedness status, and,
> if connected, do whatever setup is necessary.

Agreed.

Regards,

Anthony Liguori

>
> Adding Gerd and Markus as I think they were involved in the discussion
> last time as well.
>
> 		Amit

[Qemu-devel] [PATCH 0/3] chardev/spice: fix missing spice mouse after migration

[Alon Levy]

Adds a new char device backend callback to check connectedness, implemented
for virtio console, and used by spice-char-dev in post migration.

Is using NULL for DeviceState the intention for non device vmstates? It works
fine in practice.

Alon Levy (3):
  virtio-serial: add virtio_serial_guest_connected
  qemu-char: add qemu_chr_be_connected
  spice-qemu-char: register interface on post load

 backends/rng-egd.c      |  4 ++--
 gdbstub.c               |  2 +-
 hw/ccid-card-passthru.c |  1 +
 hw/debugcon.c           |  2 +-
 hw/ivshmem.c            |  8 ++++----
 hw/qdev-properties.c    |  2 +-
 hw/serial.c             |  4 ++--
 hw/usb/dev-serial.c     |  4 ++--
 hw/usb/redirect.c       |  2 +-
 hw/virtio-console.c     | 12 ++++++++++--
 hw/virtio-serial-bus.c  |  9 +++++++++
 hw/virtio-serial.h      |  5 +++++
 main-loop.h             |  1 +
 monitor.c               |  4 ++--
 net/slirp.c             |  2 +-
 qemu-char.c             | 11 ++++++++++-
 qemu-char.h             | 13 +++++++++++++
 qtest.c                 |  2 +-
 spice-qemu-char.c       | 34 ++++++++++++++++++++++++++++++++++
 19 files changed, 101 insertions(+), 21 deletions(-)

-- 
1.8.0

[Qemu-devel] [PATCH 1/3] virtio-serial: add virtio_serial_guest_connected

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/virtio-serial-bus.c | 9 +++++++++
 hw/virtio-serial.h     | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index ccce1fa..9147497 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -290,6 +290,15 @@ ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf,
 }
 
 /*
+ * Connectedness of the guest to the port.
+ * Returns 1 for guest connected, 0 for disconnected.
+ */
+int virtio_serial_guest_connected(VirtIOSerialPort *port)
+{
+    return port->guest_connected;
+}
+
+/*
  * Readiness of the guest to accept data on a port.
  * Returns max. data the guest can receive
  */
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 16e3982..5012194 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -194,6 +194,11 @@ ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf,
                             size_t size);
 
 /*
+ * Query whether a guest is connected.
+ */
+int virtio_serial_guest_connected(VirtIOSerialPort *port);
+
+/*
  * Query whether a guest is ready to receive data.
  */
 size_t virtio_serial_guest_ready(VirtIOSerialPort *port);
-- 
1.8.0

[Qemu-devel] [PATCH 2/3] qemu-char: add qemu_chr_be_connected

[Alon Levy]

This differs from qemu_chr_be_can_write in that it will return 1 as
long as the backend is connected, including when the backend is
connected but not available for receiving, as in the case of
virtio-serial where the guest ring may be full.

If no implementation is provided the backend reports always connected
status. This is not really important right now since there is only a
single user, spice-char-dev.c, which uses virtio-console as the backend,
which does implement the callback.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 backends/rng-egd.c      |  4 ++--
 gdbstub.c               |  2 +-
 hw/ccid-card-passthru.c |  1 +
 hw/debugcon.c           |  2 +-
 hw/ivshmem.c            |  8 ++++----
 hw/qdev-properties.c    |  2 +-
 hw/serial.c             |  4 ++--
 hw/usb/dev-serial.c     |  4 ++--
 hw/usb/redirect.c       |  2 +-
 hw/virtio-console.c     | 12 ++++++++++--
 main-loop.h             |  1 +
 monitor.c               |  4 ++--
 net/slirp.c             |  2 +-
 qemu-char.c             | 11 ++++++++++-
 qemu-char.h             | 13 +++++++++++++
 qtest.c                 |  2 +-
 16 files changed, 53 insertions(+), 21 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index ad84737..a20e737 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -150,7 +150,7 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
     }
 
     /* FIXME we should resubmit pending requests when the CDS reconnects. */
-    qemu_chr_add_handlers(s->chr, rng_egd_chr_can_read, rng_egd_chr_read,
+    qemu_chr_add_handlers(s->chr, NULL, rng_egd_chr_can_read, rng_egd_chr_read,
                           NULL, s);
 }
 
@@ -190,7 +190,7 @@ static void rng_egd_finalize(Object *obj)
     RngEgd *s = RNG_EGD(obj);
 
     if (s->chr) {
-        qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
+        qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, NULL);
     }
 
     g_free(s->chr_name);
diff --git a/gdbstub.c b/gdbstub.c
index d02ec75..6b0a2f1 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -3002,7 +3002,7 @@ int gdbserver_start(const char *device)
         if (!chr)
             return -1;
 
-        qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
+        qemu_chr_add_handlers(chr, NULL, gdb_chr_can_receive, gdb_chr_receive,
                               gdb_chr_event, NULL);
     }
 
diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index bd6c777..2734d63 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -283,6 +283,7 @@ static int passthru_initfn(CCIDCardState *base)
     if (card->cs) {
         DPRINTF(card, D_INFO, "initing chardev\n");
         qemu_chr_add_handlers(card->cs,
+            NULL,
             ccid_card_vscard_can_read,
             ccid_card_vscard_read,
             ccid_card_vscard_event, card);
diff --git a/hw/debugcon.c b/hw/debugcon.c
index 14ab326..f27ee36 100644
--- a/hw/debugcon.c
+++ b/hw/debugcon.c
@@ -73,7 +73,7 @@ static void debugcon_init_core(DebugconState *s)
         exit(1);
     }
 
-    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, s);
+    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, s);
 }
 
 static int debugcon_isa_initfn(ISADevice *dev)
diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index f6dbb21..d46ef7c 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -294,10 +294,10 @@ static CharDriverState* create_eventfd_chr_device(void * opaque, EventNotifier *
         s->eventfd_table[vector].pdev = &s->dev;
         s->eventfd_table[vector].vector = vector;
 
-        qemu_chr_add_handlers(chr, ivshmem_can_receive, fake_irqfd,
+        qemu_chr_add_handlers(chr, NULL, ivshmem_can_receive, fake_irqfd,
                       ivshmem_event, &s->eventfd_table[vector]);
     } else {
-        qemu_chr_add_handlers(chr, ivshmem_can_receive, ivshmem_receive,
+        qemu_chr_add_handlers(chr, NULL, ivshmem_can_receive, ivshmem_receive,
                       ivshmem_event, s);
     }
 
@@ -722,8 +722,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
 
         s->eventfd_chr = g_malloc0(s->vectors * sizeof(CharDriverState *));
 
-        qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read,
-                     ivshmem_event, s);
+        qemu_chr_add_handlers(s->server_chr, NULL, ivshmem_can_receive,
+                              ivshmem_read, ivshmem_event, s);
     } else {
         /* just map the file immediately, we're not using a server */
         int fd;
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 81d901c..6035128 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -550,7 +550,7 @@ static void release_chr(Object *obj, const char *name, void *opaque)
     CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
 
     if (*ptr) {
-        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
+        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL, NULL);
     }
 }
 
diff --git a/hw/serial.c b/hw/serial.c
index 60283ea..1cb8d0d 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -690,13 +690,13 @@ void serial_init_core(SerialState *s)
 
     qemu_register_reset(serial_reset, s);
 
-    qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1,
+    qemu_chr_add_handlers(s->chr, NULL, serial_can_receive1, serial_receive1,
                           serial_event, s);
 }
 
 void serial_exit_core(SerialState *s)
 {
-    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
+    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, NULL);
     qemu_unregister_reset(serial_reset, s);
 }
 
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 99b19df..5216b0c 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -414,7 +414,7 @@ static void usb_serial_handle_destroy(USBDevice *dev)
 {
     USBSerialState *s = (USBSerialState *)dev;
 
-    qemu_chr_add_handlers(s->cs, NULL, NULL, NULL, NULL);
+    qemu_chr_add_handlers(s->cs, NULL, NULL, NULL, NULL, NULL);
 }
 
 static int usb_serial_can_read(void *opaque)
@@ -491,7 +491,7 @@ static int usb_serial_initfn(USBDevice *dev)
         return -1;
     }
 
-    qemu_chr_add_handlers(s->cs, usb_serial_can_read, usb_serial_read,
+    qemu_chr_add_handlers(s->cs, NULL, usb_serial_can_read, usb_serial_read,
                           usb_serial_event, s);
     usb_serial_handle_reset(dev);
 
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 0c95e6b..cf467eb 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1082,7 +1082,7 @@ static int usbredir_initfn(USBDevice *udev)
 
     /* Let the backend know we are ready */
     qemu_chr_fe_open(dev->cs);
-    qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
+    qemu_chr_add_handlers(dev->cs, NULL, usbredir_chardev_can_read,
                           usbredir_chardev_read, usbredir_chardev_event, dev);
 
     qemu_add_vm_change_state_handler(usbredir_vm_state_change, dev);
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index cffee3d..b6022b0 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -74,6 +74,14 @@ static void guest_close(VirtIOSerialPort *port)
     qemu_chr_fe_close(vcon->chr);
 }
 
+/* Connectedness of the guest */
+static int chr_connected(void *opaque)
+{
+    VirtConsole *vcon = opaque;
+
+    return virtio_serial_guest_connected(&vcon->port);
+}
+
 /* Readiness of the guest to accept data on a port */
 static int chr_can_read(void *opaque)
 {
@@ -117,8 +125,8 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
     }
 
     if (vcon->chr) {
-        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-                              vcon);
+        qemu_chr_add_handlers(vcon->chr, chr_connected, chr_can_read, chr_read,
+                              chr_event, vcon);
     }
 
     return 0;
diff --git a/main-loop.h b/main-loop.h
index 326c742..84f5750 100644
--- a/main-loop.h
+++ b/main-loop.h
@@ -165,6 +165,7 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque);
 
 typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
 typedef int IOCanReadHandler(void *opaque);
+typedef int IOConnectedHandler(void *opaque);
 
 /**
  * qemu_set_fd_handler2: Register a file descriptor with the main loop
diff --git a/monitor.c b/monitor.c
index c0e32d6..5a74a4e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4707,13 +4707,13 @@ void monitor_init(CharDriverState *chr, int flags)
     if (monitor_ctrl_mode(mon)) {
         mon->mc = g_malloc0(sizeof(MonitorControl));
         /* Control mode requires special handlers */
-        qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
+        qemu_chr_add_handlers(chr, NULL, monitor_can_read, monitor_control_read,
                               monitor_control_event, mon);
         qemu_chr_fe_set_echo(chr, true);
 
         json_message_parser_init(&mon->mc->parser, handle_qmp_command);
     } else {
-        qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
+        qemu_chr_add_handlers(chr, NULL, monitor_can_read, monitor_read,
                               monitor_event, mon);
     }
 
diff --git a/net/slirp.c b/net/slirp.c
index afb52c3..c963bee 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -659,7 +659,7 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
         fwd->port = port;
         fwd->slirp = s->slirp;
 
-        qemu_chr_add_handlers(fwd->hd, guestfwd_can_read, guestfwd_read,
+        qemu_chr_add_handlers(fwd->hd, NULL, guestfwd_can_read, guestfwd_read,
                               NULL, fwd);
     }
     return 0;
diff --git a/qemu-char.c b/qemu-char.c
index 242b799..ba316e1 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -152,6 +152,13 @@ int qemu_chr_fe_ioctl(CharDriverState *s, int cmd, void *arg)
     return s->chr_ioctl(s, cmd, arg);
 }
 
+int qemu_chr_be_connected(CharDriverState *s)
+{
+    if (!s->chr_connected)
+        return 1;
+    return s->chr_connected(s->handler_opaque);
+}
+
 int qemu_chr_be_can_write(CharDriverState *s)
 {
     if (!s->chr_can_read)
@@ -194,6 +201,7 @@ void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
 }
 
 void qemu_chr_add_handlers(CharDriverState *s,
+                           IOConnectedHandler *fd_connected,
                            IOCanReadHandler *fd_can_read,
                            IOReadHandler *fd_read,
                            IOEventHandler *fd_event,
@@ -203,6 +211,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
         /* chr driver being released. */
         ++s->avail_connections;
     }
+    s->chr_connected = fd_connected;
     s->chr_can_read = fd_can_read;
     s->chr_read = fd_read;
     s->chr_event = fd_event;
@@ -457,7 +466,7 @@ static void mux_chr_update_read_handler(CharDriverState *chr)
     d->chr_event[d->mux_cnt] = chr->chr_event;
     /* Fix up the real driver with mux routines */
     if (d->mux_cnt == 0) {
-        qemu_chr_add_handlers(d->drv, mux_chr_can_read, mux_chr_read,
+        qemu_chr_add_handlers(d->drv, NULL, mux_chr_can_read, mux_chr_read,
                               mux_chr_event, chr);
     }
     if (d->focus != -1) {
diff --git a/qemu-char.h b/qemu-char.h
index a121e04..6e93d55 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -61,6 +61,7 @@ struct CharDriverState {
     int (*get_msgfd)(struct CharDriverState *s);
     int (*chr_add_client)(struct CharDriverState *chr, int fd);
     IOEventHandler *chr_event;
+    IOConnectedHandler *chr_connected;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
     void *handler_opaque;
@@ -202,6 +203,17 @@ int qemu_chr_fe_get_msgfd(CharDriverState *s);
 int qemu_chr_be_can_write(CharDriverState *s);
 
 /**
+ * @qemu_chr_be_connected:
+ *
+ * Determine if backend is connected.  Connected means that data will be
+ * eventually read by the backend, even if at this moment
+ * qemu_chr_be_can_write would return 0.
+ *
+ * Returns: 1 if backend is connected, 0 otherwise.
+ */
+int qemu_chr_be_connected(CharDriverState *s);
+
+/**
  * @qemu_chr_be_write:
  *
  * Write data from the back end to the front end.  Before issuing this call,
@@ -224,6 +236,7 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
 void qemu_chr_be_event(CharDriverState *s, int event);
 
 void qemu_chr_add_handlers(CharDriverState *s,
+                           IOConnectedHandler *fd_connected,
                            IOCanReadHandler *fd_can_read,
                            IOReadHandler *fd_read,
                            IOEventHandler *fd_event,
diff --git a/qtest.c b/qtest.c
index fbfab4e..c5f4193 100644
--- a/qtest.c
+++ b/qtest.c
@@ -425,7 +425,7 @@ int qtest_init(void)
     configure_icount("0");
     chr = qemu_chr_new("qtest", qtest_chrdev, NULL);
 
-    qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event, chr);
+    qemu_chr_add_handlers(chr, NULL, qtest_can_read, qtest_read, qtest_event, chr);
     qemu_chr_fe_set_echo(chr, true);
 
     inbuf = g_string_new("");
-- 
1.8.0

Re: [Qemu-devel] [PATCH 2/3] qemu-char: add qemu_chr_be_connected

[Markus Armbruster]

Alon Levy <alevy@redhat.com> writes:

> This differs from qemu_chr_be_can_write in that it will return 1 as
> long as the backend is connected, including when the backend is
> connected but not available for receiving, as in the case of
> virtio-serial where the guest ring may be full.
>
> If no implementation is provided the backend reports always connected
> status. This is not really important right now since there is only a
> single user, spice-char-dev.c, which uses virtio-console as the backend,
> which does implement the callback.
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  backends/rng-egd.c      |  4 ++--
>  gdbstub.c               |  2 +-
>  hw/ccid-card-passthru.c |  1 +
>  hw/debugcon.c           |  2 +-
>  hw/ivshmem.c            |  8 ++++----
>  hw/qdev-properties.c    |  2 +-
>  hw/serial.c             |  4 ++--
>  hw/usb/dev-serial.c     |  4 ++--
>  hw/usb/redirect.c       |  2 +-
>  hw/virtio-console.c     | 12 ++++++++++--
>  main-loop.h             |  1 +
>  monitor.c               |  4 ++--
>  net/slirp.c             |  2 +-
>  qemu-char.c             | 11 ++++++++++-
>  qemu-char.h             | 13 +++++++++++++
>  qtest.c                 |  2 +-
>  16 files changed, 53 insertions(+), 21 deletions(-)

This patch would be simpler if qemu_chr_add_handlers() took a struct
chr_handlers * pointing to static storage instead of function pointers.
I'm not asking you to make that change.

>
> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> index ad84737..a20e737 100644
> --- a/backends/rng-egd.c
> +++ b/backends/rng-egd.c
> @@ -150,7 +150,7 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
>      }
>  
>      /* FIXME we should resubmit pending requests when the CDS reconnects. */
> -    qemu_chr_add_handlers(s->chr, rng_egd_chr_can_read, rng_egd_chr_read,
> +    qemu_chr_add_handlers(s->chr, NULL, rng_egd_chr_can_read, rng_egd_chr_read,
>                            NULL, s);
>  }
>  
> @@ -190,7 +190,7 @@ static void rng_egd_finalize(Object *obj)
>      RngEgd *s = RNG_EGD(obj);
>  
>      if (s->chr) {
> -        qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
> +        qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, NULL);
>      }
>  
>      g_free(s->chr_name);
> diff --git a/gdbstub.c b/gdbstub.c
> index d02ec75..6b0a2f1 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -3002,7 +3002,7 @@ int gdbserver_start(const char *device)
>          if (!chr)
>              return -1;
>  
> -        qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
> +        qemu_chr_add_handlers(chr, NULL, gdb_chr_can_receive, gdb_chr_receive,
>                                gdb_chr_event, NULL);
>      }
>  
> diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
> index bd6c777..2734d63 100644
> --- a/hw/ccid-card-passthru.c
> +++ b/hw/ccid-card-passthru.c
> @@ -283,6 +283,7 @@ static int passthru_initfn(CCIDCardState *base)
>      if (card->cs) {
>          DPRINTF(card, D_INFO, "initing chardev\n");
>          qemu_chr_add_handlers(card->cs,
> +            NULL,
>              ccid_card_vscard_can_read,
>              ccid_card_vscard_read,
>              ccid_card_vscard_event, card);
> diff --git a/hw/debugcon.c b/hw/debugcon.c
> index 14ab326..f27ee36 100644
> --- a/hw/debugcon.c
> +++ b/hw/debugcon.c
> @@ -73,7 +73,7 @@ static void debugcon_init_core(DebugconState *s)
>          exit(1);
>      }
>  
> -    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, s);
> +    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, s);
>  }
>  
>  static int debugcon_isa_initfn(ISADevice *dev)
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index f6dbb21..d46ef7c 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -294,10 +294,10 @@ static CharDriverState* create_eventfd_chr_device(void * opaque, EventNotifier *
>          s->eventfd_table[vector].pdev = &s->dev;
>          s->eventfd_table[vector].vector = vector;
>  
> -        qemu_chr_add_handlers(chr, ivshmem_can_receive, fake_irqfd,
> +        qemu_chr_add_handlers(chr, NULL, ivshmem_can_receive, fake_irqfd,
>                        ivshmem_event, &s->eventfd_table[vector]);
>      } else {
> -        qemu_chr_add_handlers(chr, ivshmem_can_receive, ivshmem_receive,
> +        qemu_chr_add_handlers(chr, NULL, ivshmem_can_receive, ivshmem_receive,
>                        ivshmem_event, s);
>      }
>  
> @@ -722,8 +722,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>  
>          s->eventfd_chr = g_malloc0(s->vectors * sizeof(CharDriverState *));
>  
> -        qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read,
> -                     ivshmem_event, s);
> +        qemu_chr_add_handlers(s->server_chr, NULL, ivshmem_can_receive,
> +                              ivshmem_read, ivshmem_event, s);
>      } else {
>          /* just map the file immediately, we're not using a server */
>          int fd;
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 81d901c..6035128 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -550,7 +550,7 @@ static void release_chr(Object *obj, const char *name, void *opaque)
>      CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>  
>      if (*ptr) {
> -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
> +        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL, NULL);
>      }
>  }
>  
> diff --git a/hw/serial.c b/hw/serial.c
> index 60283ea..1cb8d0d 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -690,13 +690,13 @@ void serial_init_core(SerialState *s)
>  
>      qemu_register_reset(serial_reset, s);
>  
> -    qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1,
> +    qemu_chr_add_handlers(s->chr, NULL, serial_can_receive1, serial_receive1,
>                            serial_event, s);
>  }
>  
>  void serial_exit_core(SerialState *s)
>  {
> -    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
> +    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, NULL);
>      qemu_unregister_reset(serial_reset, s);
>  }
>  
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 99b19df..5216b0c 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -414,7 +414,7 @@ static void usb_serial_handle_destroy(USBDevice *dev)
>  {
>      USBSerialState *s = (USBSerialState *)dev;
>  
> -    qemu_chr_add_handlers(s->cs, NULL, NULL, NULL, NULL);
> +    qemu_chr_add_handlers(s->cs, NULL, NULL, NULL, NULL, NULL);
>  }
>  
>  static int usb_serial_can_read(void *opaque)
> @@ -491,7 +491,7 @@ static int usb_serial_initfn(USBDevice *dev)
>          return -1;
>      }
>  
> -    qemu_chr_add_handlers(s->cs, usb_serial_can_read, usb_serial_read,
> +    qemu_chr_add_handlers(s->cs, NULL, usb_serial_can_read, usb_serial_read,
>                            usb_serial_event, s);
>      usb_serial_handle_reset(dev);
>  
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 0c95e6b..cf467eb 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1082,7 +1082,7 @@ static int usbredir_initfn(USBDevice *udev)
>  
>      /* Let the backend know we are ready */
>      qemu_chr_fe_open(dev->cs);
> -    qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
> +    qemu_chr_add_handlers(dev->cs, NULL, usbredir_chardev_can_read,
>                            usbredir_chardev_read, usbredir_chardev_event, dev);
>  
>      qemu_add_vm_change_state_handler(usbredir_vm_state_change, dev);
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index cffee3d..b6022b0 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -74,6 +74,14 @@ static void guest_close(VirtIOSerialPort *port)
>      qemu_chr_fe_close(vcon->chr);
>  }
>  
> +/* Connectedness of the guest */
> +static int chr_connected(void *opaque)
> +{
> +    VirtConsole *vcon = opaque;
> +
> +    return virtio_serial_guest_connected(&vcon->port);
> +}
> +
>  /* Readiness of the guest to accept data on a port */
>  static int chr_can_read(void *opaque)
>  {
> @@ -117,8 +125,8 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
>      }
>  
>      if (vcon->chr) {
> -        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
> -                              vcon);
> +        qemu_chr_add_handlers(vcon->chr, chr_connected, chr_can_read, chr_read,
> +                              chr_event, vcon);
>      }
>  
>      return 0;
> diff --git a/main-loop.h b/main-loop.h
> index 326c742..84f5750 100644
> --- a/main-loop.h
> +++ b/main-loop.h
> @@ -165,6 +165,7 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque);
>  
>  typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
>  typedef int IOCanReadHandler(void *opaque);
> +typedef int IOConnectedHandler(void *opaque);

bool?

>  
>  /**
>   * qemu_set_fd_handler2: Register a file descriptor with the main loop
> diff --git a/monitor.c b/monitor.c
> index c0e32d6..5a74a4e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4707,13 +4707,13 @@ void monitor_init(CharDriverState *chr, int flags)
>      if (monitor_ctrl_mode(mon)) {
>          mon->mc = g_malloc0(sizeof(MonitorControl));
>          /* Control mode requires special handlers */
> -        qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
> +        qemu_chr_add_handlers(chr, NULL, monitor_can_read, monitor_control_read,
>                                monitor_control_event, mon);
>          qemu_chr_fe_set_echo(chr, true);
>  
>          json_message_parser_init(&mon->mc->parser, handle_qmp_command);
>      } else {
> -        qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
> +        qemu_chr_add_handlers(chr, NULL, monitor_can_read, monitor_read,
>                                monitor_event, mon);
>      }
>  
> diff --git a/net/slirp.c b/net/slirp.c
> index afb52c3..c963bee 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -659,7 +659,7 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
>          fwd->port = port;
>          fwd->slirp = s->slirp;
>  
> -        qemu_chr_add_handlers(fwd->hd, guestfwd_can_read, guestfwd_read,
> +        qemu_chr_add_handlers(fwd->hd, NULL, guestfwd_can_read, guestfwd_read,
>                                NULL, fwd);
>      }
>      return 0;
> diff --git a/qemu-char.c b/qemu-char.c
> index 242b799..ba316e1 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -152,6 +152,13 @@ int qemu_chr_fe_ioctl(CharDriverState *s, int cmd, void *arg)
>      return s->chr_ioctl(s, cmd, arg);
>  }
>  
> +int qemu_chr_be_connected(CharDriverState *s)
> +{
> +    if (!s->chr_connected)
> +        return 1;
> +    return s->chr_connected(s->handler_opaque);
> +}
> +
>  int qemu_chr_be_can_write(CharDriverState *s)
>  {
>      if (!s->chr_can_read)
> @@ -194,6 +201,7 @@ void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
>  }
>  
>  void qemu_chr_add_handlers(CharDriverState *s,
> +                           IOConnectedHandler *fd_connected,
>                             IOCanReadHandler *fd_can_read,
>                             IOReadHandler *fd_read,
>                             IOEventHandler *fd_event,
> @@ -203,6 +211,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
>          /* chr driver being released. */
>          ++s->avail_connections;
>      }
> +    s->chr_connected = fd_connected;
>      s->chr_can_read = fd_can_read;
>      s->chr_read = fd_read;
>      s->chr_event = fd_event;
> @@ -457,7 +466,7 @@ static void mux_chr_update_read_handler(CharDriverState *chr)
>      d->chr_event[d->mux_cnt] = chr->chr_event;
>      /* Fix up the real driver with mux routines */
>      if (d->mux_cnt == 0) {
> -        qemu_chr_add_handlers(d->drv, mux_chr_can_read, mux_chr_read,
> +        qemu_chr_add_handlers(d->drv, NULL, mux_chr_can_read, mux_chr_read,
>                                mux_chr_event, chr);
>      }
>      if (d->focus != -1) {
> diff --git a/qemu-char.h b/qemu-char.h
> index a121e04..6e93d55 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -61,6 +61,7 @@ struct CharDriverState {
>      int (*get_msgfd)(struct CharDriverState *s);
>      int (*chr_add_client)(struct CharDriverState *chr, int fd);
>      IOEventHandler *chr_event;
> +    IOConnectedHandler *chr_connected;
>      IOCanReadHandler *chr_can_read;
>      IOReadHandler *chr_read;
>      void *handler_opaque;
> @@ -202,6 +203,17 @@ int qemu_chr_fe_get_msgfd(CharDriverState *s);
>  int qemu_chr_be_can_write(CharDriverState *s);
>  
>  /**
> + * @qemu_chr_be_connected:
> + *
> + * Determine if backend is connected.  Connected means that data will be
> + * eventually read by the backend, even if at this moment
> + * qemu_chr_be_can_write would return 0.

Actually, the connected backend may disconnect without reading any more
data, but I understand what you're trying to say here.  Anybody got a a
more carefully worded explanation?

> + *
> + * Returns: 1 if backend is connected, 0 otherwise.
> + */
> +int qemu_chr_be_connected(CharDriverState *s);
> +
> +/**
>   * @qemu_chr_be_write:
>   *
>   * Write data from the back end to the front end.  Before issuing this call,
> @@ -224,6 +236,7 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
>  void qemu_chr_be_event(CharDriverState *s, int event);
>  
>  void qemu_chr_add_handlers(CharDriverState *s,
> +                           IOConnectedHandler *fd_connected,
>                             IOCanReadHandler *fd_can_read,
>                             IOReadHandler *fd_read,
>                             IOEventHandler *fd_event,
> diff --git a/qtest.c b/qtest.c
> index fbfab4e..c5f4193 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -425,7 +425,7 @@ int qtest_init(void)
>      configure_icount("0");
>      chr = qemu_chr_new("qtest", qtest_chrdev, NULL);
>  
> -    qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event, chr);
> +    qemu_chr_add_handlers(chr, NULL, qtest_can_read, qtest_read, qtest_event, chr);
>      qemu_chr_fe_set_echo(chr, true);
>  
>      inbuf = g_string_new("");

[Qemu-devel] [PATCH 3/3] spice-qemu-char: register interface on post load

[Alon Levy]

The target has not seen the guest_connected event via
spice_chr_guest_open or spice_chr_write, and so spice server wrongly
assumes there is no agent active, while the client continues to send
motion events only by the agent channel, which the server ignores. The
net effect is that the mouse is static in the guest.

By registering the interface on post load spice server will pass on the
agent messages fixing the mouse behavior after migration.

RHBZ #725965

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 spice-qemu-char.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 09aa22d..08b6ba0 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -1,6 +1,7 @@
 #include "config-host.h"
 #include "trace.h"
 #include "ui/qemu-spice.h"
+#include "hw/virtio-serial.h"
 #include <spice.h>
 #include <spice-experimental.h>
 
@@ -25,6 +26,7 @@ typedef struct SpiceCharDriver {
     uint8_t               *datapos;
     ssize_t               bufsize, datalen;
     uint32_t              debug;
+    QEMUTimer             *post_load_timer;
 } SpiceCharDriver;
 
 static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len)
@@ -156,6 +158,7 @@ static void spice_chr_close(struct CharDriverState *chr)
 
     printf("%s\n", __func__);
     vmc_unregister_interface(s);
+    qemu_free_timer(s->post_load_timer);
     g_free(s);
 }
 
@@ -188,6 +191,33 @@ static void print_allowed_subtypes(void)
     fprintf(stderr, "\n");
 }
 
+static void spice_chr_post_load_cb(void *opaque)
+{
+    SpiceCharDriver *s = opaque;
+
+    vmc_register_interface(s);
+}
+
+static int spice_chr_post_load(void *opaque, int version_id)
+{
+    SpiceCharDriver *s = opaque;
+
+    if (s && s->chr && qemu_chr_be_connected(s->chr)) {
+        qemu_mod_timer(s->post_load_timer, 1);
+    }
+    return 0;
+}
+
+static VMStateDescription spice_chr_vmstate = {
+    .name               = "spice-chr",
+    .version_id         = 1,
+    .minimum_version_id = 1,
+    .post_load          = spice_chr_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
 {
     CharDriverState *chr;
@@ -220,12 +250,16 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
     s->debug = debug;
     s->active = false;
     s->sin.subtype = subtype;
+    s->post_load_timer = qemu_new_timer_ns(vm_clock,
+                                           spice_chr_post_load_cb, s);
     chr->opaque = s;
     chr->chr_write = spice_chr_write;
     chr->chr_close = spice_chr_close;
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
 
+    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
+
 #if SPICE_SERVER_VERSION < 0x000901
     /* See comment in vmc_state() */
     if (strcmp(subtype, "vdagent") == 0) {
-- 
1.8.0

Re: [Qemu-devel] [PATCH 3/3] spice-qemu-char: register interface on post load

[Markus Armbruster]

Alon Levy <alevy@redhat.com> writes:

> The target has not seen the guest_connected event via
> spice_chr_guest_open or spice_chr_write, and so spice server wrongly
> assumes there is no agent active, while the client continues to send
> motion events only by the agent channel, which the server ignores. The
> net effect is that the mouse is static in the guest.
>
> By registering the interface on post load spice server will pass on the
> agent messages fixing the mouse behavior after migration.
>
> RHBZ #725965
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  spice-qemu-char.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 09aa22d..08b6ba0 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -1,6 +1,7 @@
>  #include "config-host.h"
>  #include "trace.h"
>  #include "ui/qemu-spice.h"
> +#include "hw/virtio-serial.h"
>  #include <spice.h>
>  #include <spice-experimental.h>
>  
> @@ -25,6 +26,7 @@ typedef struct SpiceCharDriver {
>      uint8_t               *datapos;
>      ssize_t               bufsize, datalen;
>      uint32_t              debug;
> +    QEMUTimer             *post_load_timer;
>  } SpiceCharDriver;
>  
>  static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len)
> @@ -156,6 +158,7 @@ static void spice_chr_close(struct CharDriverState *chr)
>  
>      printf("%s\n", __func__);
>      vmc_unregister_interface(s);
> +    qemu_free_timer(s->post_load_timer);
>      g_free(s);
>  }
>  
> @@ -188,6 +191,33 @@ static void print_allowed_subtypes(void)
>      fprintf(stderr, "\n");
>  }
>  
> +static void spice_chr_post_load_cb(void *opaque)
> +{
> +    SpiceCharDriver *s = opaque;
> +
> +    vmc_register_interface(s);
> +}
> +
> +static int spice_chr_post_load(void *opaque, int version_id)
> +{
> +    SpiceCharDriver *s = opaque;
> +
> +    if (s && s->chr && qemu_chr_be_connected(s->chr)) {
> +        qemu_mod_timer(s->post_load_timer, 1);
> +    }

You use the time to delay spice_chr_post_load_cb(), right?  Can you
explain why you have to delay?

> +    return 0;
> +}
> +
> +static VMStateDescription spice_chr_vmstate = {
> +    .name               = "spice-chr",
> +    .version_id         = 1,
> +    .minimum_version_id = 1,
> +    .post_load          = spice_chr_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
>  {
>      CharDriverState *chr;
> @@ -220,12 +250,16 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
>      s->debug = debug;
>      s->active = false;
>      s->sin.subtype = subtype;
> +    s->post_load_timer = qemu_new_timer_ns(vm_clock,
> +                                           spice_chr_post_load_cb, s);
>      chr->opaque = s;
>      chr->chr_write = spice_chr_write;
>      chr->chr_close = spice_chr_close;
>      chr->chr_guest_open = spice_chr_guest_open;
>      chr->chr_guest_close = spice_chr_guest_close;
>  
> +    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
> +
>  #if SPICE_SERVER_VERSION < 0x000901
>      /* See comment in vmc_state() */
>      if (strcmp(subtype, "vdagent") == 0) {

Re: [Qemu-devel] [PATCH 3/3] spice-qemu-char: register interface on post load

[Alon Levy]

> Alon Levy <alevy@redhat.com> writes:
> 
> > The target has not seen the guest_connected event via
> > spice_chr_guest_open or spice_chr_write, and so spice server
> > wrongly
> > assumes there is no agent active, while the client continues to
> > send
> > motion events only by the agent channel, which the server ignores.
> > The
> > net effect is that the mouse is static in the guest.
> >
> > By registering the interface on post load spice server will pass on
> > the
> > agent messages fixing the mouse behavior after migration.
> >
> > RHBZ #725965
> >
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  spice-qemu-char.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> > index 09aa22d..08b6ba0 100644
> > --- a/spice-qemu-char.c
> > +++ b/spice-qemu-char.c
> > @@ -1,6 +1,7 @@
> >  #include "config-host.h"
> >  #include "trace.h"
> >  #include "ui/qemu-spice.h"
> > +#include "hw/virtio-serial.h"
> >  #include <spice.h>
> >  #include <spice-experimental.h>
> >  
> > @@ -25,6 +26,7 @@ typedef struct SpiceCharDriver {
> >      uint8_t               *datapos;
> >      ssize_t               bufsize, datalen;
> >      uint32_t              debug;
> > +    QEMUTimer             *post_load_timer;
> >  } SpiceCharDriver;
> >  
> >  static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t
> >  *buf, int len)
> > @@ -156,6 +158,7 @@ static void spice_chr_close(struct
> > CharDriverState *chr)
> >  
> >      printf("%s\n", __func__);
> >      vmc_unregister_interface(s);
> > +    qemu_free_timer(s->post_load_timer);
> >      g_free(s);
> >  }
> >  
> > @@ -188,6 +191,33 @@ static void print_allowed_subtypes(void)
> >      fprintf(stderr, "\n");
> >  }
> >  
> > +static void spice_chr_post_load_cb(void *opaque)
> > +{
> > +    SpiceCharDriver *s = opaque;
> > +
> > +    vmc_register_interface(s);
> > +}
> > +
> > +static int spice_chr_post_load(void *opaque, int version_id)
> > +{
> > +    SpiceCharDriver *s = opaque;
> > +
> > +    if (s && s->chr && qemu_chr_be_connected(s->chr)) {
> > +        qemu_mod_timer(s->post_load_timer, 1);
> > +    }
> 
> You use the time to delay spice_chr_post_load_cb(), right?  Can you
> explain why you have to delay?

This is a precaution, it ensures vmc_register_interface is called when the vm is running as opposed to stopped which is the state when spice_chr_post_load is called. In theory vmc_register_interface could lead to an attempt to inject an interrupt into the guest, and we know that fails with kvm irqchip from a previous bug with virtio-serial.

> 
> > +    return 0;
> > +}
> > +
> > +static VMStateDescription spice_chr_vmstate = {
> > +    .name               = "spice-chr",
> > +    .version_id         = 1,
> > +    .minimum_version_id = 1,
> > +    .post_load          = spice_chr_post_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> >  CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
> >  {
> >      CharDriverState *chr;
> > @@ -220,12 +250,16 @@ CharDriverState *qemu_chr_open_spice(QemuOpts
> > *opts)
> >      s->debug = debug;
> >      s->active = false;
> >      s->sin.subtype = subtype;
> > +    s->post_load_timer = qemu_new_timer_ns(vm_clock,
> > +                                           spice_chr_post_load_cb,
> > s);
> >      chr->opaque = s;
> >      chr->chr_write = spice_chr_write;
> >      chr->chr_close = spice_chr_close;
> >      chr->chr_guest_open = spice_chr_guest_open;
> >      chr->chr_guest_close = spice_chr_guest_close;
> >  
> > +    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
> > +
> >  #if SPICE_SERVER_VERSION < 0x000901
> >      /* See comment in vmc_state() */
> >      if (strcmp(subtype, "vdagent") == 0) {
> 
> 

Re: [Qemu-devel] [PATCH 3/3] spice-qemu-char: register interface on post load

[Markus Armbruster]

Alon Levy <alevy@redhat.com> writes:

>> Alon Levy <alevy@redhat.com> writes:
>> 
>> > The target has not seen the guest_connected event via
>> > spice_chr_guest_open or spice_chr_write, and so spice server
>> > wrongly
>> > assumes there is no agent active, while the client continues to
>> > send
>> > motion events only by the agent channel, which the server ignores.
>> > The
>> > net effect is that the mouse is static in the guest.
>> >
>> > By registering the interface on post load spice server will pass on
>> > the
>> > agent messages fixing the mouse behavior after migration.
>> >
>> > RHBZ #725965
>> >
>> > Signed-off-by: Alon Levy <alevy@redhat.com>
>> > ---
>> >  spice-qemu-char.c | 34 ++++++++++++++++++++++++++++++++++
>> >  1 file changed, 34 insertions(+)
>> >
>> > diff --git a/spice-qemu-char.c b/spice-qemu-char.c
>> > index 09aa22d..08b6ba0 100644
>> > --- a/spice-qemu-char.c
>> > +++ b/spice-qemu-char.c
>> > @@ -1,6 +1,7 @@
>> >  #include "config-host.h"
>> >  #include "trace.h"
>> >  #include "ui/qemu-spice.h"
>> > +#include "hw/virtio-serial.h"
>> >  #include <spice.h>
>> >  #include <spice-experimental.h>
>> >  
>> > @@ -25,6 +26,7 @@ typedef struct SpiceCharDriver {
>> >      uint8_t               *datapos;
>> >      ssize_t               bufsize, datalen;
>> >      uint32_t              debug;
>> > +    QEMUTimer             *post_load_timer;
>> >  } SpiceCharDriver;
>> >  
>> >  static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t
>> >  *buf, int len)
>> > @@ -156,6 +158,7 @@ static void spice_chr_close(struct
>> > CharDriverState *chr)
>> >  
>> >      printf("%s\n", __func__);
>> >      vmc_unregister_interface(s);
>> > +    qemu_free_timer(s->post_load_timer);
>> >      g_free(s);
>> >  }
>> >  
>> > @@ -188,6 +191,33 @@ static void print_allowed_subtypes(void)
>> >      fprintf(stderr, "\n");
>> >  }
>> >  
>> > +static void spice_chr_post_load_cb(void *opaque)
>> > +{
>> > +    SpiceCharDriver *s = opaque;
>> > +
>> > +    vmc_register_interface(s);
>> > +}
>> > +
>> > +static int spice_chr_post_load(void *opaque, int version_id)
>> > +{
>> > +    SpiceCharDriver *s = opaque;
>> > +
>> > +    if (s && s->chr && qemu_chr_be_connected(s->chr)) {
>> > +        qemu_mod_timer(s->post_load_timer, 1);
>> > +    }
>> 
>> You use the time to delay spice_chr_post_load_cb(), right?  Can you
>> explain why you have to delay?
>
> This is a precaution, it ensures vmc_register_interface is called when
> the vm is running as opposed to stopped which is the state when
> spice_chr_post_load is called. In theory vmc_register_interface could
> lead to an attempt to inject an interrupt into the guest, and we know
> that fails with kvm irqchip from a previous bug with virtio-serial.

So your fixed delay of 1ns (I think) on the vm_clock is a roundabout way
to delay the callback from "post load" until after the run state
transition to RUN_STATE_RUNNING.  Correct?

If yes, then a VM change state handler might be cleaner.

[...]

Re: [Qemu-devel] [PATCH 3/3] spice-qemu-char: register interface on post load

[Alon Levy]

> Alon Levy <alevy@redhat.com> writes:
> 
> >> Alon Levy <alevy@redhat.com> writes:
> >> 
> >> > The target has not seen the guest_connected event via
> >> > spice_chr_guest_open or spice_chr_write, and so spice server
> >> > wrongly
> >> > assumes there is no agent active, while the client continues to
> >> > send
> >> > motion events only by the agent channel, which the server
> >> > ignores.
> >> > The
> >> > net effect is that the mouse is static in the guest.
> >> >
> >> > By registering the interface on post load spice server will pass
> >> > on
> >> > the
> >> > agent messages fixing the mouse behavior after migration.
> >> >
> >> > RHBZ #725965
> >> >
> >> > Signed-off-by: Alon Levy <alevy@redhat.com>
> >> > ---
> >> >  spice-qemu-char.c | 34 ++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 34 insertions(+)
> >> >
> >> > diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> >> > index 09aa22d..08b6ba0 100644
> >> > --- a/spice-qemu-char.c
> >> > +++ b/spice-qemu-char.c
> >> > @@ -1,6 +1,7 @@
> >> >  #include "config-host.h"
> >> >  #include "trace.h"
> >> >  #include "ui/qemu-spice.h"
> >> > +#include "hw/virtio-serial.h"
> >> >  #include <spice.h>
> >> >  #include <spice-experimental.h>
> >> >  
> >> > @@ -25,6 +26,7 @@ typedef struct SpiceCharDriver {
> >> >      uint8_t               *datapos;
> >> >      ssize_t               bufsize, datalen;
> >> >      uint32_t              debug;
> >> > +    QEMUTimer             *post_load_timer;
> >> >  } SpiceCharDriver;
> >> >  
> >> >  static int vmc_write(SpiceCharDeviceInstance *sin, const
> >> >  uint8_t
> >> >  *buf, int len)
> >> > @@ -156,6 +158,7 @@ static void spice_chr_close(struct
> >> > CharDriverState *chr)
> >> >  
> >> >      printf("%s\n", __func__);
> >> >      vmc_unregister_interface(s);
> >> > +    qemu_free_timer(s->post_load_timer);
> >> >      g_free(s);
> >> >  }
> >> >  
> >> > @@ -188,6 +191,33 @@ static void print_allowed_subtypes(void)
> >> >      fprintf(stderr, "\n");
> >> >  }
> >> >  
> >> > +static void spice_chr_post_load_cb(void *opaque)
> >> > +{
> >> > +    SpiceCharDriver *s = opaque;
> >> > +
> >> > +    vmc_register_interface(s);
> >> > +}
> >> > +
> >> > +static int spice_chr_post_load(void *opaque, int version_id)
> >> > +{
> >> > +    SpiceCharDriver *s = opaque;
> >> > +
> >> > +    if (s && s->chr && qemu_chr_be_connected(s->chr)) {
> >> > +        qemu_mod_timer(s->post_load_timer, 1);
> >> > +    }
> >> 
> >> You use the time to delay spice_chr_post_load_cb(), right?  Can
> >> you
> >> explain why you have to delay?
> >
> > This is a precaution, it ensures vmc_register_interface is called
> > when
> > the vm is running as opposed to stopped which is the state when
> > spice_chr_post_load is called. In theory vmc_register_interface
> > could
> > lead to an attempt to inject an interrupt into the guest, and we
> > know
> > that fails with kvm irqchip from a previous bug with virtio-serial.
> 
> So your fixed delay of 1ns (I think) on the vm_clock is a roundabout
> way
> to delay the callback from "post load" until after the run state
> transition to RUN_STATE_RUNNING.  Correct?
Yes.

> 
> If yes, then a VM change state handler might be cleaner.

Not saying that two wrongs make a right, but this is also the way we handled the irq injection in post_load for virtio serial console:

 http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg01196.html

> 
> [...]
> 

Re: [Qemu-devel] [PATCH 3/3] spice-qemu-char: register interface on post load

[Amit Shah]

On (Wed) 28 Nov 2012 [12:59:40], Markus Armbruster wrote:
> Alon Levy <alevy@redhat.com> writes:
> 
> >> Alon Levy <alevy@redhat.com> writes:
> >> 
> >> > The target has not seen the guest_connected event via
> >> > spice_chr_guest_open or spice_chr_write, and so spice server
> >> > wrongly
> >> > assumes there is no agent active, while the client continues to
> >> > send
> >> > motion events only by the agent channel, which the server ignores.
> >> > The
> >> > net effect is that the mouse is static in the guest.
> >> >
> >> > By registering the interface on post load spice server will pass on
> >> > the
> >> > agent messages fixing the mouse behavior after migration.
> >> >
> >> > RHBZ #725965
> >> >
> >> > Signed-off-by: Alon Levy <alevy@redhat.com>
> >> > ---
> >> >  spice-qemu-char.c | 34 ++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 34 insertions(+)
> >> >
> >> > diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> >> > index 09aa22d..08b6ba0 100644
> >> > --- a/spice-qemu-char.c
> >> > +++ b/spice-qemu-char.c
> >> > @@ -1,6 +1,7 @@
> >> >  #include "config-host.h"
> >> >  #include "trace.h"
> >> >  #include "ui/qemu-spice.h"
> >> > +#include "hw/virtio-serial.h"
> >> >  #include <spice.h>
> >> >  #include <spice-experimental.h>
> >> >  
> >> > @@ -25,6 +26,7 @@ typedef struct SpiceCharDriver {
> >> >      uint8_t               *datapos;
> >> >      ssize_t               bufsize, datalen;
> >> >      uint32_t              debug;
> >> > +    QEMUTimer             *post_load_timer;
> >> >  } SpiceCharDriver;
> >> >  
> >> >  static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t
> >> >  *buf, int len)
> >> > @@ -156,6 +158,7 @@ static void spice_chr_close(struct
> >> > CharDriverState *chr)
> >> >  
> >> >      printf("%s\n", __func__);
> >> >      vmc_unregister_interface(s);
> >> > +    qemu_free_timer(s->post_load_timer);
> >> >      g_free(s);
> >> >  }
> >> >  
> >> > @@ -188,6 +191,33 @@ static void print_allowed_subtypes(void)
> >> >      fprintf(stderr, "\n");
> >> >  }
> >> >  
> >> > +static void spice_chr_post_load_cb(void *opaque)
> >> > +{
> >> > +    SpiceCharDriver *s = opaque;
> >> > +
> >> > +    vmc_register_interface(s);
> >> > +}
> >> > +
> >> > +static int spice_chr_post_load(void *opaque, int version_id)
> >> > +{
> >> > +    SpiceCharDriver *s = opaque;
> >> > +
> >> > +    if (s && s->chr && qemu_chr_be_connected(s->chr)) {
> >> > +        qemu_mod_timer(s->post_load_timer, 1);
> >> > +    }
> >> 
> >> You use the time to delay spice_chr_post_load_cb(), right?  Can you
> >> explain why you have to delay?
> >
> > This is a precaution, it ensures vmc_register_interface is called when
> > the vm is running as opposed to stopped which is the state when
> > spice_chr_post_load is called. In theory vmc_register_interface could
> > lead to an attempt to inject an interrupt into the guest, and we know
> > that fails with kvm irqchip from a previous bug with virtio-serial.
> 
> So your fixed delay of 1ns (I think) on the vm_clock is a roundabout way
> to delay the callback from "post load" until after the run state
> transition to RUN_STATE_RUNNING.  Correct?
> 
> If yes, then a VM change state handler might be cleaner.

Agreed.  Juan and I had discussed this earlier, and there are no hooks
on the target side (i.e. for loadvm) yet.  So this roundabout way is
the best way to solve the problem for now.

(This discussion with Juan was done earlier, when the patch to
virtio-serial-bus pointed to by Alon in the other message was done).

		Amit

Re: [Qemu-devel] [PATCH 0/3] chardev/spice: fix missing spice mouse after migration

[Amit Shah]

On (Wed) 28 Nov 2012 [11:05:37], Alon Levy wrote:
> Adds a new char device backend callback to check connectedness, implemented
> for virtio console, and used by spice-char-dev in post migration.
> 
> Is using NULL for DeviceState the intention for non device vmstates? It works
> fine in practice.

Any more opinions / reviews on this series?  Anthony?

		Amit

Re: [Qemu-devel] [PATCH 0/3] chardev/spice: fix missing spice mouse after migration

[Anthony Liguori]

Amit Shah <amit.shah@redhat.com> writes:

> On (Wed) 28 Nov 2012 [11:05:37], Alon Levy wrote:
>> Adds a new char device backend callback to check connectedness, implemented
>> for virtio console, and used by spice-char-dev in post migration.
>> 
>> Is using NULL for DeviceState the intention for non device vmstates? It works
>> fine in practice.
>
> Any more opinions / reviews on this series?  Anthony?

Spice should save its state and not rely on the chardev layer to replay
it.

I thought this was already pointed out in this thread?

Regards,

Anthony Liguori

>
> 		Amit

Re: [Qemu-devel] [PATCH 0/3] chardev/spice: fix missing spice mouse after migration

[Amit Shah]

On (Thu) 13 Dec 2012 [08:25:08], Anthony Liguori wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Wed) 28 Nov 2012 [11:05:37], Alon Levy wrote:
> >> Adds a new char device backend callback to check connectedness, implemented
> >> for virtio console, and used by spice-char-dev in post migration.
> >> 
> >> Is using NULL for DeviceState the intention for non device vmstates? It works
> >> fine in practice.
> >
> > Any more opinions / reviews on this series?  Anthony?
> 
> Spice should save its state and not rely on the chardev layer to replay
> it.
> 
> I thought this was already pointed out in this thread?

Indeed.  Alon, just adding save/load to spice-qemu-char.c doesn't work
for you?

		Amit

[Qemu-devel] [PATCH] spice-qemu-char: register interface on post load

[Alon Levy]

The target has not seen the guest_connected event via
spice_chr_guest_open or spice_chr_write, and so spice server wrongly
assumes there is no agent active, while the client continues to send
motion events only by the agent channel, which the server ignores. The
net effect is that the mouse is static in the guest.

By registering the interface on post load spice server will pass on the
agent messages fixing the mouse behavior after migration.

RHBZ #725965

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 spice-qemu-char.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index a4d7de8..e6eb523 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -2,6 +2,7 @@
 #include "trace.h"
 #include "ui/qemu-spice.h"
 #include "char/char.h"
+#include "migration/vmstate.h"
 #include <spice.h>
 #include <spice-experimental.h>
 #include <spice/protocol.h>
@@ -26,6 +27,10 @@ typedef struct SpiceCharDriver {
     ssize_t               bufsize, datalen;
     uint32_t              debug;
     QLIST_ENTRY(SpiceCharDriver) next;
+    uint32_t              guest_open;
+    struct {
+        QEMUTimer        *timer;
+    } post_load;
 } SpiceCharDriver;
 
 static QLIST_HEAD(, SpiceCharDriver) spice_chars =
@@ -185,18 +190,23 @@ static void spice_chr_close(struct CharDriverState *chr)
     printf("%s\n", __func__);
     vmc_unregister_interface(s);
     QLIST_REMOVE(s, next);
+    qemu_free_timer(s->post_load.timer);
     g_free(s);
 }
 
 static void spice_chr_guest_open(struct CharDriverState *chr)
 {
     SpiceCharDriver *s = chr->opaque;
+
+    s->guest_open = 1;
     vmc_register_interface(s);
 }
 
 static void spice_chr_guest_close(struct CharDriverState *chr)
 {
     SpiceCharDriver *s = chr->opaque;
+
+    s->guest_open = 0;
     vmc_unregister_interface(s);
 }
 
@@ -217,6 +227,34 @@ static void print_allowed_subtypes(void)
     fprintf(stderr, "\n");
 }
 
+static void spice_chr_post_load_cb(void *opaque)
+{
+    SpiceCharDriver *s = opaque;
+
+    spice_chr_guest_open(s->chr);
+}
+
+static int spice_chr_post_load(void *opaque, int version_id)
+{
+    SpiceCharDriver *s = opaque;
+
+    if (s && s->chr && s->guest_open) {
+        qemu_mod_timer(s->post_load.timer, 1);
+    }
+    return 0;
+}
+
+static VMStateDescription spice_chr_vmstate = {
+    .name               = "spice-chr",
+    .version_id         = 1,
+    .minimum_version_id = 1,
+    .post_load          = spice_chr_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(guest_open, SpiceCharDriver),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static CharDriverState *chr_open(QemuOpts *opts, const char *subtype)
 {
     CharDriverState *chr;
@@ -229,12 +267,16 @@ static CharDriverState *chr_open(QemuOpts *opts, const char *subtype)
     s->debug = debug;
     s->active = false;
     s->sin.subtype = subtype;
+    s->post_load.timer = qemu_new_timer_ns(vm_clock,
+                                           spice_chr_post_load_cb, s);
     chr->opaque = s;
     chr->chr_write = spice_chr_write;
     chr->chr_close = spice_chr_close;
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
 
+    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
+
     QLIST_INSERT_HEAD(&spice_chars, s, next);
 
     return chr;
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH] spice-qemu-char: register interface on post load

[Amit Shah]

On (Sun) 23 Dec 2012 [23:35:29], Alon Levy wrote:
> The target has not seen the guest_connected event via
> spice_chr_guest_open or spice_chr_write, and so spice server wrongly
> assumes there is no agent active, while the client continues to send
> motion events only by the agent channel, which the server ignores. The
> net effect is that the mouse is static in the guest.
> 
> By registering the interface on post load spice server will pass on the
> agent messages fixing the mouse behavior after migration.
> 
> RHBZ #725965
> 
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  spice-qemu-char.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)

I suppose Gerd should pick this up in his tree?

I have a couple of questions below, but I ack this approach.

>  static QLIST_HEAD(, SpiceCharDriver) spice_chars =
> @@ -185,18 +190,23 @@ static void spice_chr_close(struct CharDriverState *chr)
>      printf("%s\n", __func__);
>      vmc_unregister_interface(s);
>      QLIST_REMOVE(s, next);
> +    qemu_free_timer(s->post_load.timer);

Also vmstate_unregister()?

I'm wondering if there can be a case where this function is called
before the timer has had a chance to fire.  It can happen if the spice
port is hot-unplugged before the guest has had a chance to run on the
target.  In that case, qemu_del_timer() should be called as well, to
ensure the timer doesn't fire with invalid args later.

		Amit