[PATCH RFC 0/4] vdagent: support live migration

[PATCH RFC 0/4] vdagent: support live migration

[dengpc12]

From: "dengpc12@chinatelecom.cn" <dengpc12@chinatelecom.cn>

1. after live migration, copy/paste with vnc is not working. this is because:
1). vd->caps is not saved; this will leads wrong clipboard type is prased in vdagent_clipboard_recv_grab;
2). vdagent isn`t register to qemu-clipboard; this will leads vdagent cannot send/receive messages from qemu-clipboard, thus copy
validated on win2016 data center evaluation with spice-guest-tools-latest(spice-guest-tools-0.141) and on fedora 37 with spice-vd

2. the possible memory leak
we called qemu_input_handler_register, but not calling qemu_input_handler_unregister; 
qemu_input_handler_register will mallocate memory;

we post this patchset for RFC and any corrections and suggetions about
the implementation are very appreciated!

Please review, thanks !

Best Regards !

dengpc12@chinatelecom.cn (4):
  vdagent: fix memory leak when vdagent_disconnect is called
  vdagent: refactor vdagent_chr_recv_caps function
  vdagent: add live migration support
  vdagent: remove migration blocker

 ui/trace-events |  2 ++
 ui/vdagent.c    | 62 +++++++++++++++++++++++++++++++++----------------
 2 files changed, 44 insertions(+), 20 deletions(-)

-- 
2.27.0

[PATCH RFC 1/4] vdagent: fix memory leak when vdagent_disconnect is called

[dengpc12]

From: "dengpc12@chinatelecom.cn" <dengpc12@chinatelecom.cn>

Memory free should be done in vdagent_disconnect using
qemu_input_handler_unregister, replace qemu_input_handler_deactivate
with that.

Signed-off-by: dengpc12@chinatelecom.cn <dengpc12@chinatelecom.cn>
Signed-off-by: liuym16@chinatelecom.cn <liuym16@chinatelecom.cn>
---
 ui/vdagent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/vdagent.c b/ui/vdagent.c
index 4bf50f0c4d..645383b4ec 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -863,7 +863,7 @@ static void vdagent_disconnect(VDAgentChardev *vd)
     vdagent_reset_bufs(vd);
     vd->caps = 0;
     if (vd->mouse_hs) {
-        qemu_input_handler_deactivate(vd->mouse_hs);
+        qemu_input_handler_unregister(vd->mouse_hs);
     }
     if (vd->cbpeer.notifier.notify) {
         qemu_clipboard_peer_unregister(&vd->cbpeer);
-- 
2.27.0

[PATCH RFC 2/4] vdagent: refactor vdagent_chr_recv_caps function

[dengpc12]

From: "dengpc12@chinatelecom.cn" <dengpc12@chinatelecom.cn>

Abstract vdagent registry logic into
vdagent_register_to_qemu_clipboard.

Note that trace log of vdagent_recv_caps also be added.

Signed-off-by: dengpc12@chinatelecom.cn <dengpc12@chinatelecom.cn>
Signed-off-by: liuym16@chinatelecom.cn <liuym16@chinatelecom.cn>
---
 ui/trace-events |  1 +
 ui/vdagent.c    | 20 +++++++++++++-------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/ui/trace-events b/ui/trace-events
index 977577fbba..5e50b60da5 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -143,6 +143,7 @@ vdagent_cb_grab_selection(const char *name) "selection %s"
 vdagent_cb_grab_discard(const char *name, int cur, int recv) "selection %s, cur:%d recv:%d"
 vdagent_cb_grab_type(const char *name) "type %s"
 vdagent_cb_serial_discard(uint32_t current, uint32_t received) "current=%u, received=%u"
+vdagent_recv_caps(uint32_t caps) "received caps %u"
 
 # dbus.c
 dbus_registered_listener(const char *bus_name) "peer %s"
diff --git a/ui/vdagent.c b/ui/vdagent.c
index 645383b4ec..38061d5b38 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -696,6 +696,16 @@ static void vdagent_chr_open(Chardev *chr,
     *be_opened = true;
 }
 
+static void vdagent_register_to_qemu_clipboard(VDAgentChardev *vd)
+{
+    if (have_clipboard(vd) && vd->cbpeer.notifier.notify == NULL) {
+        vd->cbpeer.name = "vdagent";
+        vd->cbpeer.notifier.notify = vdagent_clipboard_notify;
+        vd->cbpeer.request = vdagent_clipboard_request;
+        qemu_clipboard_peer_register(&vd->cbpeer);
+    }
+}
+
 static void vdagent_chr_recv_caps(VDAgentChardev *vd, VDAgentMessage *msg)
 {
     VDAgentAnnounceCapabilities *caps = (void *)msg->data;
@@ -720,14 +730,10 @@ static void vdagent_chr_recv_caps(VDAgentChardev *vd, VDAgentMessage *msg)
         qemu_input_handler_activate(vd->mouse_hs);
     }
 
-    memset(vd->last_serial, 0, sizeof(vd->last_serial));
+    trace_vdagent_recv_caps(vd->caps);
 
-    if (have_clipboard(vd) && vd->cbpeer.notifier.notify == NULL) {
-        vd->cbpeer.name = "vdagent";
-        vd->cbpeer.notifier.notify = vdagent_clipboard_notify;
-        vd->cbpeer.request = vdagent_clipboard_request;
-        qemu_clipboard_peer_register(&vd->cbpeer);
-    }
+    memset(vd->last_serial, 0, sizeof(vd->last_serial));
+    vdagent_register_to_qemu_clipboard(vd);
 }
 
 static void vdagent_chr_recv_msg(VDAgentChardev *vd, VDAgentMessage *msg)
-- 
2.27.0

[PATCH RFC 3/4] vdagent: add live migration support

[dengpc12]

From: "dengpc12@chinatelecom.cn" <dengpc12@chinatelecom.cn>

To support live migration, we made the following 2 modifications:
1. save the caps field of VDAgentChardev.
2. register vdagent to qemu-clipboard after
   vm device state being reloaded during live migration.

Signed-off-by: dengpc12@chinatelecom.cn <dengpc12@chinatelecom.cn>
Signed-off-by: liuym16@chinatelecom.cn <liuym16@chinatelecom.cn>
---
 ui/trace-events |  1 +
 ui/vdagent.c    | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/ui/trace-events b/ui/trace-events
index 5e50b60da5..ccacd867d1 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -144,6 +144,7 @@ vdagent_cb_grab_discard(const char *name, int cur, int recv) "selection %s, cur:
 vdagent_cb_grab_type(const char *name) "type %s"
 vdagent_cb_serial_discard(uint32_t current, uint32_t received) "current=%u, received=%u"
 vdagent_recv_caps(uint32_t caps) "received caps %u"
+vdagent_migration_caps(uint32_t caps) "migrated caps %u"
 
 # dbus.c
 dbus_registered_listener(const char *bus_name) "peer %s"
diff --git a/ui/vdagent.c b/ui/vdagent.c
index 38061d5b38..1193abe348 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -6,6 +6,7 @@
 #include "qemu/units.h"
 #include "hw/qdev-core.h"
 #include "migration/blocker.h"
+#include "migration/vmstate.h"
 #include "ui/clipboard.h"
 #include "ui/console.h"
 #include "ui/input.h"
@@ -906,6 +907,31 @@ static void vdagent_chr_parse(QemuOpts *opts, ChardevBackend *backend,
 
 /* ------------------------------------------------------------------ */
 
+static int vdagent_post_load(void *opaque, int version_id)
+{
+    VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(opaque);
+
+    trace_vdagent_migration_caps(vd->caps);
+
+    if (vd->caps) {
+        vdagent_register_to_qemu_clipboard(vd);
+        qemu_input_handler_activate(vd->mouse_hs);
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_vdagent = {
+    .name = "vdagent",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = vdagent_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(caps, VDAgentChardev),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static void vdagent_chr_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
@@ -922,6 +948,8 @@ static void vdagent_chr_init(Object *obj)
     VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
 
     buffer_init(&vd->outbuf, "vdagent-outbuf");
+
+    vmstate_register(NULL, 0, &vmstate_vdagent, vd);
     error_setg(&vd->migration_blocker,
                "The vdagent chardev doesn't yet support migration");
 }
-- 
2.27.0

[PATCH RFC 4/4] vdagent: remove migration blocker

[dengpc12]

From: "dengpc12@chinatelecom.cn" <dengpc12@chinatelecom.cn>

Now that migration already be supported, so remove the blocker.

Signed-off-by: dengpc12@chinatelecom.cn <dengpc12@chinatelecom.cn>
Signed-off-by: liuym16@chinatelecom.cn <liuym16@chinatelecom.cn>
---
 ui/vdagent.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/ui/vdagent.c b/ui/vdagent.c
index 1193abe348..f0a7fd5093 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -5,7 +5,6 @@
 #include "qemu/option.h"
 #include "qemu/units.h"
 #include "hw/qdev-core.h"
-#include "migration/blocker.h"
 #include "migration/vmstate.h"
 #include "ui/clipboard.h"
 #include "ui/console.h"
@@ -32,9 +31,6 @@
 struct VDAgentChardev {
     Chardev parent;
 
-    /* TODO: migration isn't yet supported */
-    Error *migration_blocker;
-
     /* config */
     bool mouse;
     bool clipboard;
@@ -675,10 +671,6 @@ static void vdagent_chr_open(Chardev *chr,
     return;
 #endif
 
-    if (migrate_add_blocker(vd->migration_blocker, errp) != 0) {
-        return;
-    }
-
     vd->mouse = VDAGENT_MOUSE_DEFAULT;
     if (cfg->has_mouse) {
         vd->mouse = cfg->mouse;
@@ -950,18 +942,14 @@ static void vdagent_chr_init(Object *obj)
     buffer_init(&vd->outbuf, "vdagent-outbuf");
 
     vmstate_register(NULL, 0, &vmstate_vdagent, vd);
-    error_setg(&vd->migration_blocker,
-               "The vdagent chardev doesn't yet support migration");
 }
 
 static void vdagent_chr_fini(Object *obj)
 {
     VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
 
-    migrate_del_blocker(vd->migration_blocker);
     vdagent_disconnect(vd);
     buffer_free(&vd->outbuf);
-    error_free(vd->migration_blocker);
 }
 
 static const TypeInfo vdagent_chr_type_info = {
-- 
2.27.0

Re: [PATCH RFC 1/4] vdagent: fix memory leak when vdagent_disconnect is called

[Marc-André Lureau]

Hi

On Fri, Dec 30, 2022 at 6:48 PM <dengpc12@chinatelecom.cn> wrote:
>
> From: "dengpc12@chinatelecom.cn" <dengpc12@chinatelecom.cn>
>
> Memory free should be done in vdagent_disconnect using
> qemu_input_handler_unregister, replace qemu_input_handler_deactivate
> with that.
>
> Signed-off-by: dengpc12@chinatelecom.cn <dengpc12@chinatelecom.cn>
> Signed-off-by: liuym16@chinatelecom.cn <liuym16@chinatelecom.cn>
> ---
>  ui/vdagent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ui/vdagent.c b/ui/vdagent.c
> index 4bf50f0c4d..645383b4ec 100644
> --- a/ui/vdagent.c
> +++ b/ui/vdagent.c
> @@ -863,7 +863,7 @@ static void vdagent_disconnect(VDAgentChardev *vd)
>      vdagent_reset_bufs(vd);
>      vd->caps = 0;
>      if (vd->mouse_hs) {
> -        qemu_input_handler_deactivate(vd->mouse_hs);
> +        qemu_input_handler_unregister(vd->mouse_hs);

It looks like vdagent_disconnect() was meant to be called when the
agent connection is lost, vdagent_chr_set_fe_open(fe_open=false), so
it can later be initialized again during vdagent_chr_recv_caps(). Not
sure why this isn't done.

Imho, you should instead add qemu_input_handler_unregister() call to
vdagent_chr_fini() for now


-- 
Marc-André Lureau

Re: [PATCH RFC 2/4] vdagent: refactor vdagent_chr_recv_caps function

[Marc-André Lureau]

On Fri, Dec 30, 2022 at 6:49 PM <dengpc12@chinatelecom.cn> wrote:
>
> From: "dengpc12@chinatelecom.cn" <dengpc12@chinatelecom.cn>
>
> Abstract vdagent registry logic into
> vdagent_register_to_qemu_clipboard.
>
> Note that trace log of vdagent_recv_caps also be added.
>
> Signed-off-by: dengpc12@chinatelecom.cn <dengpc12@chinatelecom.cn>
> Signed-off-by: liuym16@chinatelecom.cn <liuym16@chinatelecom.cn>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  ui/trace-events |  1 +
>  ui/vdagent.c    | 20 +++++++++++++-------
>  2 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/ui/trace-events b/ui/trace-events
> index 977577fbba..5e50b60da5 100644
> --- a/ui/trace-events
> +++ b/ui/trace-events
> @@ -143,6 +143,7 @@ vdagent_cb_grab_selection(const char *name) "selection %s"
>  vdagent_cb_grab_discard(const char *name, int cur, int recv) "selection %s, cur:%d recv:%d"
>  vdagent_cb_grab_type(const char *name) "type %s"
>  vdagent_cb_serial_discard(uint32_t current, uint32_t received) "current=%u, received=%u"
> +vdagent_recv_caps(uint32_t caps) "received caps %u"
>
>  # dbus.c
>  dbus_registered_listener(const char *bus_name) "peer %s"
> diff --git a/ui/vdagent.c b/ui/vdagent.c
> index 645383b4ec..38061d5b38 100644
> --- a/ui/vdagent.c
> +++ b/ui/vdagent.c
> @@ -696,6 +696,16 @@ static void vdagent_chr_open(Chardev *chr,
>      *be_opened = true;
>  }
>
> +static void vdagent_register_to_qemu_clipboard(VDAgentChardev *vd)
> +{
> +    if (have_clipboard(vd) && vd->cbpeer.notifier.notify == NULL) {
> +        vd->cbpeer.name = "vdagent";
> +        vd->cbpeer.notifier.notify = vdagent_clipboard_notify;
> +        vd->cbpeer.request = vdagent_clipboard_request;
> +        qemu_clipboard_peer_register(&vd->cbpeer);
> +    }
> +}
> +
>  static void vdagent_chr_recv_caps(VDAgentChardev *vd, VDAgentMessage *msg)
>  {
>      VDAgentAnnounceCapabilities *caps = (void *)msg->data;
> @@ -720,14 +730,10 @@ static void vdagent_chr_recv_caps(VDAgentChardev *vd, VDAgentMessage *msg)
>          qemu_input_handler_activate(vd->mouse_hs);
>      }
>
> -    memset(vd->last_serial, 0, sizeof(vd->last_serial));
> +    trace_vdagent_recv_caps(vd->caps);
>
> -    if (have_clipboard(vd) && vd->cbpeer.notifier.notify == NULL) {
> -        vd->cbpeer.name = "vdagent";
> -        vd->cbpeer.notifier.notify = vdagent_clipboard_notify;
> -        vd->cbpeer.request = vdagent_clipboard_request;
> -        qemu_clipboard_peer_register(&vd->cbpeer);
> -    }
> +    memset(vd->last_serial, 0, sizeof(vd->last_serial));
> +    vdagent_register_to_qemu_clipboard(vd);
>  }
>
>  static void vdagent_chr_recv_msg(VDAgentChardev *vd, VDAgentMessage *msg)
> --
> 2.27.0
>
>


-- 
Marc-André Lureau

Re: [PATCH RFC 3/4] vdagent: add live migration support

[Marc-André Lureau]

Hi


On Fri, Dec 30, 2022 at 6:49 PM <dengpc12@chinatelecom.cn> wrote:
>
> From: "dengpc12@chinatelecom.cn" <dengpc12@chinatelecom.cn>
>
> To support live migration, we made the following 2 modifications:
> 1. save the caps field of VDAgentChardev.
> 2. register vdagent to qemu-clipboard after
>    vm device state being reloaded during live migration.
>
> Signed-off-by: dengpc12@chinatelecom.cn <dengpc12@chinatelecom.cn>
> Signed-off-by: liuym16@chinatelecom.cn <liuym16@chinatelecom.cn>
> ---
>  ui/trace-events |  1 +
>  ui/vdagent.c    | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/ui/trace-events b/ui/trace-events
> index 5e50b60da5..ccacd867d1 100644
> --- a/ui/trace-events
> +++ b/ui/trace-events
> @@ -144,6 +144,7 @@ vdagent_cb_grab_discard(const char *name, int cur, int recv) "selection %s, cur:
>  vdagent_cb_grab_type(const char *name) "type %s"
>  vdagent_cb_serial_discard(uint32_t current, uint32_t received) "current=%u, received=%u"
>  vdagent_recv_caps(uint32_t caps) "received caps %u"
> +vdagent_migration_caps(uint32_t caps) "migrated caps %u"
>
>  # dbus.c
>  dbus_registered_listener(const char *bus_name) "peer %s"
> diff --git a/ui/vdagent.c b/ui/vdagent.c
> index 38061d5b38..1193abe348 100644
> --- a/ui/vdagent.c
> +++ b/ui/vdagent.c
> @@ -6,6 +6,7 @@
>  #include "qemu/units.h"
>  #include "hw/qdev-core.h"
>  #include "migration/blocker.h"
> +#include "migration/vmstate.h"
>  #include "ui/clipboard.h"
>  #include "ui/console.h"
>  #include "ui/input.h"
> @@ -906,6 +907,31 @@ static void vdagent_chr_parse(QemuOpts *opts, ChardevBackend *backend,
>
>  /* ------------------------------------------------------------------ */
>
> +static int vdagent_post_load(void *opaque, int version_id)
> +{
> +    VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(opaque);
> +
> +    trace_vdagent_migration_caps(vd->caps);
> +
> +    if (vd->caps) {
> +        vdagent_register_to_qemu_clipboard(vd);
> +        qemu_input_handler_activate(vd->mouse_hs);
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_vdagent = {
> +    .name = "vdagent",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = vdagent_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(caps, VDAgentChardev),

You are missing a lot of states from VDAgentChardev. Most of the
fields must be saved/restored.

> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static void vdagent_chr_class_init(ObjectClass *oc, void *data)
>  {
>      ChardevClass *cc = CHARDEV_CLASS(oc);
> @@ -922,6 +948,8 @@ static void vdagent_chr_init(Object *obj)
>      VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
>
>      buffer_init(&vd->outbuf, "vdagent-outbuf");
> +
> +    vmstate_register(NULL, 0, &vmstate_vdagent, vd);
>      error_setg(&vd->migration_blocker,
>                 "The vdagent chardev doesn't yet support migration");
>  }
> --
> 2.27.0
>
>


--
Marc-André Lureau