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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

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