Hi
On Wed, Oct 18, 2023 at 5:38 PM Edmund Raile <edmund.ra...@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.ra...@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
