Hi On Mon, Jul 28, 2025 at 5:06 PM startergo <starte...@protonmail.com> wrote: > > Subject: Re: [PATCH RFC v1 1/1] ui/sdl2: clipboard sharing implementation for > SDL > In-Reply-To: > <cakmqykm+4kbqynhkqwxqphyp1zn5crtxc4r7yj8wbx5m+tc...@mail.gmail.com> > References: <20231108105411.1759509-1-ad...@xutaxkamay.com> > <cakmqykm+4kbqynhkqwxqphyp1zn5crtxc4r7yj8wbx5m+tc...@mail.gmail.com> > > Hi Marc-André, > > Following up on your thoughtful review of the SDL clipboard RFC from November > 2023, > I've developed a comprehensive implementation that directly addresses the > concerns > you raised about main loop reentrancy and clipboard management issues. > > ## Key Improvements Addressing Your Feedback: > > **1. Main Loop Reentrancy Solution** > You correctly identified the problematic `main_loop_wait(false)` pattern from > the > original RFC. My implementation eliminates this entirely by: > - Using immediate data processing without busy-wait loops > - Implementing proper asynchronous clipboard handling > - Following the same safety patterns used in QEMU issue #1150 resolution > > **2. Clipboard Manager Conflict Prevention** > Your concern about fighting with clipboard managers is addressed through: > - Self-update loop prevention in `sdl2_clipboard_update()` > - Clean ownership tracking via `info->owner == &scon->cbpeer` checks > - No automatic clipboard stealing or aggressive management behavior > > **3. Null Termination Handling** > Regarding your question about proper string handling: my implementation > ensures: > - SDL-compatible null-terminated UTF-8 strings using `g_strndup()` > - Proper length calculation excluding null terminator for QEMU clipboard > - Safe handling of embedded nulls in clipboard data > > **4. Configuration Options** > Following your suggestion about the optional nature (like gtk_clipboard), the > implementation includes: > - `CONFIG_SDL_CLIPBOARD` build option for conditional compilation > - Clean fallback when clipboard support is disabled > - No forced dependencies or runtime requirements > > ## Technical Implementation Details: > > The implementation uses event-driven clipboard monitoring via > `SDL_CLIPBOARDUPDATE` > rather than polling, and integrates cleanly with QEMU's unified clipboard > subsystem > through the `QemuClipboardPeer` interface. > > Key safety features: > - No main loop reentrancy > - Proper memory management with SDL-specific allocation/deallocation > - Self-update prevention to avoid clipboard ownership conflicts > - UTF-8 string validation and proper null termination > > ## Testing and Validation: > > Extensive testing on macOS with Linux guest demonstrates: > - Reliable bidirectional clipboard operation > - No performance impact or stability regressions > - Clean coexistence with system clipboard managers > - Proper handling of various text encodings and formats > > This implementation addresses the SDL2 backend's clipboard gap while > incorporating > lessons learned from both the GTK clipboard implementation and the community > feedback from the original RFC. > > The patch brings SDL2 to feature parity with other QEMU display backends > regarding > clipboard functionality, using a safety-first approach that avoids the > pitfalls > identified in your review. > > Would appreciate your thoughts on this refined approach. The implementation is > ready for community review and addresses the architectural concerns raised in > the original thread. > > Best regards, > startergo > > --- > > [Complete patch follows below] >
Please send a properly formatted git patch: https://www.qemu.org/docs/master/devel/submitting-a-patch.html#do-not-send-as-an-attachment > From: startergo <starte...@protonmail.com> > Date: Mon, 28 Jul 2025 12:00:00 +0000 > Subject: [PATCH] ui/sdl2: Add bidirectional clipboard support > > This patch implements bidirectional clipboard functionality for the SDL2 > display backend, addressing the lack of clipboard integration when using > SDL2 as the display interface. > > The implementation addresses concerns raised in previous SDL clipboard > discussions, particularly around main loop reentrancy and clipboard > manager conflicts identified in the November 2023 RFC review.Key features: > - Bidirectional text clipboard synchronization between host and guest > - Safe implementation avoiding main loop reentrancy issues > - Proper memory management with SDL-specific allocation/deallocation > - Integration with QEMU's unified clipboard subsystem > - Configurable via CONFIG_SDL_CLIPBOARD build option The patch is missing meson updates for a new "sdl_clipboard" option. It would not be required if you can avoid the main loop reentrancy. You removed it, but I am afraid you didn't address the issue from Kamay's original patch. > > The implementation follows established QEMU patterns and addresses > reentrancy concerns similar to those resolved in QEMU issue #1150. > > Implementation details: > - Uses SDL_CLIPBOARDUPDATE events to detect host clipboard changes > - Implements QemuClipboardPeer interface for guest-to-host direction > - Avoids busy-wait loops by processing clipboard data immediately > - Proper UTF-8 handling following SDL2 string conventions > - Memory management uses SDL_free() for SDL-allocated strings > - Self-update prevention to avoid clipboard manager conflicts > > The patch has been tested extensively on macOS with various guest > operating systems including Linux and Windows. Clipboard functionality > works reliably in both directions without performance impact or > stability issues. > > This addresses a significant usability gap in the SDL2 backend, bringing > it to feature parity with other QEMU display backends regarding clipboard > functionality. > > Signed-off-by: startergo <starte...@protonmail.com> > --- > ui/meson.build | 1 + > include/ui/sdl2.h | 11 +++ > ui/clipboard.c | 104 ++++++++++++++++++++++++++++++++++++++++++ name it sdl2-clipboard.c > ui/sdl2.c | 8 ++++ > 4 files changed, 124 insertions(+) > create mode 100644 ui/clipboard.c > > diff --git a/ui/meson.build b/ui/meson.build > index 92e7e61219..c5e7880ca5 100644 > --- a/ui/meson.build > +++ b/ui/meson.build > @@ -59,6 +59,7 @@ if sdl.found() > softmmu_ss.add(when: 'CONFIG_SDL', if_true: files( > 'sdl2-2d.c', > 'sdl2-gl.c', > + 'clipboard.c', make it conditional on have_sdl_clipboard option > 'sdl2-input.c', > 'sdl2.c' > )) > diff --git a/include/ui/sdl2.h b/include/ui/sdl2.h > index 1624ad6938..28a17e7b53 100644 > --- a/include/ui/sdl2.h > +++ b/include/ui/sdl2.h > @@ -7,6 +7,10 @@ > # include <SDL.h> > # include <SDL_syswm.h> > > +#ifdef CONFIG_SDL_CLIPBOARD > +#include "ui/clipboard.h" > +#endif > + > struct sdl2_console { > DisplayChangeListener dcl; > DisplaySurface *surface; > @@ -22,6 +26,10 @@ struct sdl2_console { > int idle_counter; > int ignore_hotkeys; > SDL_GLContext winctx; > + > +#ifdef CONFIG_SDL_CLIPBOARD > + QemuClipboardPeer cbpeer; > +#endif > }; > > void sdl2_window_create(struct sdl2_console *scon); > @@ -39,4 +47,7 @@ void sdl2_reset_keys(struct sdl2_console *scon); > void sdl2_process_key(struct sdl2_console *scon, > SDL_KeyboardEvent *ev); > > +void sdl2_clipboard_init(struct sdl2_console *scon); > +void sdl2_clipboard_handle_request(struct sdl2_console *scon); > + > #endif /* SDL2_H */ > diff --git a/ui/clipboard.c b/ui/clipboard.c > new file mode 100644 > index 0000000000..98fa9f1c2a > --- /dev/null > +++ b/ui/clipboard.c > @@ -0,0 +1,104 @@ > +/* > + * QEMU SDL2 clipboard implementation > + * > + * Copyright (c) 2025 startergo > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > copy > + * of this software and associated documentation files (the "Software"), to > deal > + * in the Software without restriction, including without limitation the > rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ QEMU code has SPDX identifiers and is GPL2+: /* * Copyright ... * * SPDX-License-Identifier: GPL-2.0-or-later */ > + > +#include "qemu/osdep.h" > +#include "ui/sdl2.h" > + > +#ifdef CONFIG_SDL_CLIPBOARD > + > +static void sdl2_clipboard_update(struct sdl2_console *scon, > + QemuClipboardInfo *info) > +{ > + QemuClipboardType type = QEMU_CLIPBOARD_TYPE_TEXT; > + g_autoptr(QemuClipboardData) data = NULL; > + > + /* Prevent self-update loops */ > + if (info->owner == &scon->cbpeer) { > + return; > + } > + > + /* Only handle text clipboard for now */ > + if (!qemu_clipboard_info_has_type(info, type)) { > + return; > + } > + > + /* Get clipboard data from QEMU */ > + data = qemu_clipboard_request(info, type); > + if (!data || !data->data || data->size == 0) { > + return; > + } Here, Kamay's patch was waiting for the clipboard to be filled. You can't assume the data is readily available after calling qemu_clipboard_request(). vdagent code will send a request, and data can come later asynchronously via VD_AGENT_CLIPBOARD message. The code must deal with QEMU_CLIPBOARD_UPDATE_INFO notifiers / callbacks and handle request state tracking to properly deal with this. > + > + /* > + * Ensure null termination for SDL clipboard. > + * QEMU clipboard data may not be null-terminated. > + */ > + g_autofree char *text = g_strndup((const char *)data->data, data->size); casting required here? > + if (text) { text can't be NULL if data->data is non-NULL. If we want to handle the case anyway, we could have a trace or a warning > + SDL_SetClipboardText(text); > + } > +} > + > +static void sdl2_clipboard_notify(Notifier *notifier, > + void *data) > +{ > + QemuClipboardNotify *notify = data; > + struct sdl2_console *scon = > + container_of(notifier, struct sdl2_console, cbpeer.notifier); > + > + switch (notify->type) { > + case QEMU_CLIPBOARD_UPDATE_INFO: > + sdl2_clipboard_update(scon, notify->info); > + break; > + case QEMU_CLIPBOARD_RESET_SERIAL: > + /* Nothing to do for reset */ > + break; > + } > +} > + > +static void sdl2_clipboard_request(QemuClipboardInfo *info, > + QemuClipboardType type) > +{ > + struct sdl2_console *scon = > + container_of(info->owner, struct sdl2_console, cbpeer); > + char *sdl_text = NULL; > + > + switch (type) { > + case QEMU_CLIPBOARD_TYPE_TEXT: > + if (!SDL_HasClipboardText()) { > + return; > + } > + > + sdl_text = SDL_GetClipboardText(); > + if (sdl_text && strlen(sdl_text) > 0) { Interesting that SDL decided that empty string is for error reporting. Could you simplify the check with sdl_text[0] != '\0' instead? Also add a warning with SDL_GetError() if it's empty. > + /* > + * SDL guarantees null-terminated UTF-8 strings. > + * Pass length without null terminator as QEMU clipboard > + * will handle null termination consistently. > + */ > + qemu_clipboard_set_data(&scon->cbpeer, info, type, > + strlen(sdl_text), sdl_text, true); > + } > + > + /* Always free SDL-allocated memory */ > + if (sdl_text) { drop the condition, GetClipboardText() should not return NULL, and SDL_free(NULL) is fine anyway. > + SDL_free(sdl_text); > + } > + break; > + default: > + break; > + } > +} > + > +void sdl2_clipboard_handle_request(struct sdl2_console *scon) > +{ > + g_autoptr(QemuClipboardInfo) info = > + qemu_clipboard_info_new(&scon->cbpeer, > + QEMU_CLIPBOARD_SELECTION_CLIPBOARD); > + > + if (info) { qemu_clipboard_info_new never returns NULL > + sdl2_clipboard_request(info, QEMU_CLIPBOARD_TYPE_TEXT); > + } > +} > + > +void sdl2_clipboard_init(struct sdl2_console *scon) > +{ > + scon->cbpeer.name = "sdl2"; > + scon->cbpeer.notifier.notify = sdl2_clipboard_notify; > + scon->cbpeer.request = sdl2_clipboard_request; > + > + /* Register the clipboard peer with QEMU */ > + qemu_clipboard_peer_register(&scon->cbpeer); > +} > + > +#endif /* CONFIG_SDL_CLIPBOARD */ > diff --git a/ui/sdl2.c b/ui/sdl2.c > index 1a83c3b1bf..5678930d3c 100644 > --- a/ui/sdl2.c > +++ b/ui/sdl2.c > @@ -691,6 +691,11 @@ void sdl2_poll_events(struct sdl2_console *scon) > case SDL_WINDOWEVENT: > handle_windowevent(ev); > break; > +#ifdef CONFIG_SDL_CLIPBOARD > + case SDL_CLIPBOARDUPDATE: > + sdl2_clipboard_handle_request(scon); > + break; > +#endif > default: > break; > } > @@ -914,6 +919,9 @@ static void sdl2_display_init(DisplayState *ds, > DisplayOptions *o) > qemu_console_set_window_id(con, info.info.x11.window); > #endif > } > +#ifdef CONFIG_SDL_CLIPBOARD > + sdl2_clipboard_init(&sdl2_console[i]); > +#endif > } > > #ifdef CONFIG_SDL_IMAGE > -- Marc-André Lureau
