diff mbox series

[v3] ui/gtk-clipboard: async owner_change clipboard_request

Message ID 20231018133621.721259-1-edmund.raile@proton.me
State New
Headers show
Series [v3] ui/gtk-clipboard: async owner_change clipboard_request | expand

Commit Message

edmund.raile Oct. 18, 2023, 1:37 p.m. UTC 
Previous implementation of both functions was blocking and caused guest
freezes / crashes on host clipboard owner change.
 * use callbacks instead of waiting for GTK to deliver
   clipboard content type evaluation and contents
 * evaluate a serial in the info struct to discard old events

Fixes: d11ebe2ca257 ("ui/gtk: add clipboard support")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1150
Signed-off-by: Edmund Raile <edmund.raile@proton.me>
---

Gitlab user kolAflash is to credit for determining that the main issue
of the QEMU-UI-GTK clipboard is the call to the blocking function
gtk_clipboard_wait_is_text_available in gd_owner_change, causing guests
to freeze / crash when GTK takes too long.
Marc-André Lureau suggested: 
 * gd_clipboard_request might express the same issue due to using
   gtk_clipboard_wait_for_text
 * the callbacks could use the QemuClipboardInfo struct's serial field
   to discard old events

This patch implements asynchronous gd_clipboard_request and
gd_owner_change with serial checking.

What I haven't implemented is gd_clipboard_notify's
QEMU_CLIPBOARD_RESET_SERIAL handling, I don't know how to.

Please help me test this patch.
The issue mentions the conditions, so far it has been stable.
Note that you will need to build QEMU with `enable-gtk-clipboard`.
command line options for qemu-vdagent:
-device virtio-serial,packed=on,ioeventfd=on \
-device virtserialport,name=com.redhat.spice.0,chardev=vdagent0 \
-chardev qemu-vdagent,id=vdagent0,name=vdagent,clipboard=on,mouse=off \
The guests spice-vdagent user service may have to be started manually.

If testing is sufficient and shows no way to break this, we could undo
or modify 29e0bfffab87d89c65c0890607e203b1579590a3
to have the GTK UI's clipboard built-in by default again.

Previous threads:
 * https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg06027.html
 * https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg04397.html
I am not responding to either of the previous threads so as to not break
anything in the mailing list by correcting my mistake in the subject.

 ui/gtk-clipboard.c | 84 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 70 insertions(+), 14 deletions(-)

Comments

Marc-André Lureau Nov. 14, 2023, 12:19 p.m. UTC | #1 
Hi

On Wed, Oct 18, 2023 at 5:38 PM Edmund Raile <edmund.raile@proton.me> wrote:
>
> Previous implementation of both functions was blocking and caused guest
> freezes / crashes on host clipboard owner change.
>  * use callbacks instead of waiting for GTK to deliver
>    clipboard content type evaluation and contents
>  * evaluate a serial in the info struct to discard old events
>
> Fixes: d11ebe2ca257 ("ui/gtk: add clipboard support")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1150
> Signed-off-by: Edmund Raile <edmund.raile@proton.me>
> ---
>
> Gitlab user kolAflash is to credit for determining that the main issue
> of the QEMU-UI-GTK clipboard is the call to the blocking function
> gtk_clipboard_wait_is_text_available in gd_owner_change, causing guests
> to freeze / crash when GTK takes too long.
> Marc-André Lureau suggested:
>  * gd_clipboard_request might express the same issue due to using
>    gtk_clipboard_wait_for_text
>  * the callbacks could use the QemuClipboardInfo struct's serial field
>    to discard old events
>
> This patch implements asynchronous gd_clipboard_request and
> gd_owner_change with serial checking.
>
> What I haven't implemented is gd_clipboard_notify's
> QEMU_CLIPBOARD_RESET_SERIAL handling, I don't know how to.
>
> Please help me test this patch.
> The issue mentions the conditions, so far it has been stable.
> Note that you will need to build QEMU with `enable-gtk-clipboard`.
> command line options for qemu-vdagent:
> -device virtio-serial,packed=on,ioeventfd=on \
> -device virtserialport,name=com.redhat.spice.0,chardev=vdagent0 \
> -chardev qemu-vdagent,id=vdagent0,name=vdagent,clipboard=on,mouse=off \
> The guests spice-vdagent user service may have to be started manually.
>
> If testing is sufficient and shows no way to break this, we could undo
> or modify 29e0bfffab87d89c65c0890607e203b1579590a3
> to have the GTK UI's clipboard built-in by default again.
>
> Previous threads:
>  * https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg06027.html
>  * https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg04397.html
> I am not responding to either of the previous threads so as to not break
> anything in the mailing list by correcting my mistake in the subject.
>
>  ui/gtk-clipboard.c | 84 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 70 insertions(+), 14 deletions(-)
>
> diff --git a/ui/gtk-clipboard.c b/ui/gtk-clipboard.c
> index 8d8a636fd1..07fe8b0ce1 100644
> --- a/ui/gtk-clipboard.c
> +++ b/ui/gtk-clipboard.c
> @@ -133,26 +133,85 @@ static void gd_clipboard_notify(Notifier *notifier, void *data)
>      }
>  }
>
> +/*
> + * asynchronous clipboard text transfer callback
> + * called when host (gtk) is ready to deliver to guest
> + */
> +static void gd_clipboard_request_text_callback
> +    (GtkClipboard *clipboard, const gchar *text, gpointer data)
> +{
> +    QemuClipboardInfo *info = (QemuClipboardInfo *)data;
> +

No need for cast with a gpointer.

> +    if (!text || !qemu_clipboard_check_serial(info, true)) {
> +        return;
> +    }
> +
> +    qemu_clipboard_set_data(info->owner, info, QEMU_CLIPBOARD_TYPE_TEXT,
> +                            strlen(text), text, true);
> +    return;

drop that return; line

unref(info) (see below)

> +}
> +
> +/*
> + * asynchronous clipboard data transfer initiator
> + * guest requests, host delivers when ready
> + */
>  static void gd_clipboard_request(QemuClipboardInfo *info,
>                                   QemuClipboardType type)
>  {
>      GtkDisplayState *gd = container_of(info->owner, GtkDisplayState, cbpeer);
> -    char *text;
>
>      switch (type) {
>      case QEMU_CLIPBOARD_TYPE_TEXT:
> -        text = gtk_clipboard_wait_for_text(gd->gtkcb[info->selection]);
> -        if (text) {
> -            qemu_clipboard_set_data(&gd->cbpeer, info, type,
> -                                    strlen(text), text, true);
> -            g_free(text);
> -        }
> +        gtk_clipboard_request_text
> +            (gd->gtkcb[info->selection],
> +             gd_clipboard_request_text_callback, info);

You should ref() info here

>          break;
>      default:
>          break;
>      }
>  }
>
> +/*
> + * asynchronous clipboard text availability notification callback
> + * called when host (gtk) is ready to notify guest
> + */
> +static void gd_owner_change_text_callback
> +    (GtkClipboard *clipboard, const gchar *text, gpointer data)
> +{
> +    QemuClipboardInfo *info = (QemuClipboardInfo *)data;
> +
> +    static uint32_t notification_serial_last;
> +
> +    /*
> +     * performing the subtraction of uints as ints
> +     * is a neat trick to guard against rollover issues
> +     */
> +    if (!text ||
> +        (((int32_t)(info->serial - notification_serial_last)) <= 0))

You should only handle the last update, so a simple comparison with a
GtkDisplayState clipboard_request_serial field should do.

> +    {
> +        goto end;
> +    }
> +
> +    notification_serial_last = info->serial;
> +
> +    info->types[QEMU_CLIPBOARD_TYPE_TEXT].available = true;
> +    qemu_clipboard_update(info);
> +
> +    goto end;

drop that line

> +
> +end:
> +    /*
> +     * this notification info struct is temporary
> +     * and can safely be freed after use
> +     */
> +    qemu_clipboard_info_unref(info);
> +    return;

needless return;

> +}
> +
> +/*
> + * asynchronous clipboard data availability notification initiator
> + * host notifies guest when ready
> + */
>  static void gd_owner_change(GtkClipboard *clipboard,
>                              GdkEvent *event,
>                              gpointer data)
> @@ -160,22 +219,19 @@ static void gd_owner_change(GtkClipboard *clipboard,
>      GtkDisplayState *gd = data;
>      QemuClipboardSelection s = gd_find_selection(gd, clipboard);
>      QemuClipboardInfo *info;
> +    static uint32_t notification_serial;

You should use a GtkDisplayState field instead.
>
>      if (gd->cbowner[s]) {
>          /* ignore notifications about our own grabs */
>          return;
>      }
>
> -
>      switch (event->owner_change.reason) {
>      case GDK_OWNER_CHANGE_NEW_OWNER:
>          info = qemu_clipboard_info_new(&gd->cbpeer, s);
> -        if (gtk_clipboard_wait_is_text_available(clipboard)) {
> -            info->types[QEMU_CLIPBOARD_TYPE_TEXT].available = true;
> -        }
> -
> -        qemu_clipboard_update(info);
> -        qemu_clipboard_info_unref(info);
> +        info->serial = ++notification_serial;
> +        gtk_clipboard_request_text
> +            (clipboard, gd_owner_change_text_callback, info);
>          break;
>      default:
>          qemu_clipboard_peer_release(&gd->cbpeer, s);
> --
> 2.42.0
>
>
>


--
Marc-André Lureau
diff mbox series

Patch

diff --git a/ui/gtk-clipboard.c b/ui/gtk-clipboard.c
index 8d8a636fd1..07fe8b0ce1 100644
--- a/ui/gtk-clipboard.c
+++ b/ui/gtk-clipboard.c
@@ -133,26 +133,85 @@  static void gd_clipboard_notify(Notifier *notifier, void *data)
     }
 }
 
+/*
+ * asynchronous clipboard text transfer callback
+ * called when host (gtk) is ready to deliver to guest
+ */
+static void gd_clipboard_request_text_callback
+    (GtkClipboard *clipboard, const gchar *text, gpointer data)
+{
+    QemuClipboardInfo *info = (QemuClipboardInfo *)data;
+
+    if (!text || !qemu_clipboard_check_serial(info, true)) {
+        return;
+    }
+
+    qemu_clipboard_set_data(info->owner, info, QEMU_CLIPBOARD_TYPE_TEXT,
+                            strlen(text), text, true);
+    return;
+}
+
+/*
+ * asynchronous clipboard data transfer initiator
+ * guest requests, host delivers when ready
+ */
 static void gd_clipboard_request(QemuClipboardInfo *info,
                                  QemuClipboardType type)
 {
     GtkDisplayState *gd = container_of(info->owner, GtkDisplayState, cbpeer);
-    char *text;
 
     switch (type) {
     case QEMU_CLIPBOARD_TYPE_TEXT:
-        text = gtk_clipboard_wait_for_text(gd->gtkcb[info->selection]);
-        if (text) {
-            qemu_clipboard_set_data(&gd->cbpeer, info, type,
-                                    strlen(text), text, true);
-            g_free(text);
-        }
+        gtk_clipboard_request_text
+            (gd->gtkcb[info->selection],
+             gd_clipboard_request_text_callback, info);
         break;
     default:
         break;
     }
 }
 
+/*
+ * asynchronous clipboard text availability notification callback
+ * called when host (gtk) is ready to notify guest
+ */
+static void gd_owner_change_text_callback
+    (GtkClipboard *clipboard, const gchar *text, gpointer data)
+{
+    QemuClipboardInfo *info = (QemuClipboardInfo *)data;
+
+    static uint32_t notification_serial_last;
+
+    /*
+     * performing the subtraction of uints as ints
+     * is a neat trick to guard against rollover issues
+     */
+    if (!text ||
+        (((int32_t)(info->serial - notification_serial_last)) <= 0))
+    {
+        goto end;
+    }
+
+    notification_serial_last = info->serial;
+
+    info->types[QEMU_CLIPBOARD_TYPE_TEXT].available = true;
+    qemu_clipboard_update(info);
+
+    goto end;
+
+end:
+    /*
+     * this notification info struct is temporary
+     * and can safely be freed after use
+     */
+    qemu_clipboard_info_unref(info);
+    return;
+}
+
+/*
+ * asynchronous clipboard data availability notification initiator
+ * host notifies guest when ready
+ */
 static void gd_owner_change(GtkClipboard *clipboard,
                             GdkEvent *event,
                             gpointer data)
@@ -160,22 +219,19 @@  static void gd_owner_change(GtkClipboard *clipboard,
     GtkDisplayState *gd = data;
     QemuClipboardSelection s = gd_find_selection(gd, clipboard);
     QemuClipboardInfo *info;
+    static uint32_t notification_serial;
 
     if (gd->cbowner[s]) {
         /* ignore notifications about our own grabs */
         return;
     }
 
-
     switch (event->owner_change.reason) {
     case GDK_OWNER_CHANGE_NEW_OWNER:
         info = qemu_clipboard_info_new(&gd->cbpeer, s);
-        if (gtk_clipboard_wait_is_text_available(clipboard)) {
-            info->types[QEMU_CLIPBOARD_TYPE_TEXT].available = true;
-        }
-
-        qemu_clipboard_update(info);
-        qemu_clipboard_info_unref(info);
+        info->serial = ++notification_serial;
+        gtk_clipboard_request_text
+            (clipboard, gd_owner_change_text_callback, info);
         break;
     default:
         qemu_clipboard_peer_release(&gd->cbpeer, s);