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

[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>

v2:
 call qemu_del_timer, unregister vmstate
---
 spice-qemu-char.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 54 insertions(+), 10 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index aea3d24..5f81d0e 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 =
@@ -178,25 +183,19 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
     return len;
 }
 
-static void spice_chr_close(struct CharDriverState *chr)
-{
-    SpiceCharDriver *s = chr->opaque;
-
-    printf("%s\n", __func__);
-    vmc_unregister_interface(s);
-    QLIST_REMOVE(s, next);
-    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 +216,47 @@ 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 void spice_chr_close(struct CharDriverState *chr)
+{
+    SpiceCharDriver *s = chr->opaque;
+
+    printf("%s\n", __func__);
+    vmc_unregister_interface(s);
+    QLIST_REMOVE(s, next);
+    qemu_del_timer(s->post_load.timer);
+    qemu_free_timer(s->post_load.timer);
+    vmstate_unregister(NULL, &spice_chr_vmstate, s);
+    g_free(s);
+}
+
 static CharDriverState *chr_open(QemuOpts *opts, const char *subtype)
 {
     CharDriverState *chr;
@@ -229,12 +269,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.1.4

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

[Hans de Goede]

Hi,

On 03/14/2013 04:12 PM, 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

Erm I just posted a cleaned up version of this, which actually applies
on top of all the recent chardev works done. So lets use my version :)

Regards,

Hans

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

[Alon Levy]

> Hi,
> 
> On 03/14/2013 04:12 PM, 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
> 
> Erm I just posted a cleaned up version of this, which actually
> applies
> on top of all the recent chardev works done. So lets use my version
> :)

OK.

> 
> Regards,
> 
> Hans
> 

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