Hi
On Tue, Mar 24, 2026 at 6:47 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Mar 17, 2026 at 12:50:59PM +0400, Marc-André Lureau wrote:
> > Do not add the display state to the vnc list, if the initialization
> > failed. Add vnc_display_free(), to free the display state and associated
> > data in such case. The function is meant to be public and reused in the
> > following changes.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > ui/keymaps.h | 1 +
> > ui/keymaps.c | 13 ++++++++++---
> > ui/vnc.c | 30 ++++++++++++++++++++++++++----
> > 3 files changed, 37 insertions(+), 7 deletions(-)
> >
> > diff --git a/ui/keymaps.h b/ui/keymaps.h
> > index 3d52c0882a1..e8917e56404 100644
> > --- a/ui/keymaps.h
> > +++ b/ui/keymaps.h
> > @@ -54,6 +54,7 @@ typedef struct kbd_layout_t kbd_layout_t;
> >
> > kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> > const char *language, Error **errp);
> > +void kbd_layout_free(kbd_layout_t *k);
> > int keysym2scancode(kbd_layout_t *k, int keysym,
> > QKbdState *kbd, bool down);
> > int keycode_is_keypad(kbd_layout_t *k, int keycode);
> > diff --git a/ui/keymaps.c b/ui/keymaps.c
> > index 2359dbfe7e6..d1b3f43dc8a 100644
> > --- a/ui/keymaps.c
> > +++ b/ui/keymaps.c
> > @@ -178,6 +178,14 @@ out:
> > return ret;
> > }
> >
> > +void kbd_layout_free(kbd_layout_t *k)
> > +{
> > + if (!k) {
> > + return;
> > + }
> > + g_hash_table_unref(k->hash);
> > + g_free(k);
> > +}
> >
> > kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> > const char *language, Error **errp)
> > @@ -185,10 +193,9 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> > kbd_layout_t *k;
> >
> > k = g_new0(kbd_layout_t, 1);
> > - k->hash = g_hash_table_new(NULL, NULL);
> > + k->hash = g_hash_table_new_full(NULL, NULL, NULL, g_free);
> > if (parse_keyboard_layout(k, table, language, errp) < 0) {
> > - g_hash_table_unref(k->hash);
> > - g_free(k);
> > + kbd_layout_free(k);
> > return NULL;
> > }
> > return k;
>
> This is fixing a memory leak in init_keyboard_layout that's separate
> from the VNC leak, so these ui/keymaps.c should be their own commit.
ok
>
>
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 763b13acbde..115ff8a988e 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -3421,6 +3421,8 @@ static void vmstate_change_handler(void *opaque, bool running, RunState state)
> > update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_BASE);
> > }
> >
> > +static void vnc_display_free(VncDisplay *vd);
> > +
> > void vnc_display_init(const char *id, Error **errp)
> > {
> > VncDisplay *vd;
> > @@ -3430,8 +3432,9 @@ void vnc_display_init(const char *id, Error **errp)
> > }
> > vd = g_malloc0(sizeof(*vd));
> >
> > + qemu_mutex_init(&vd->mutex);
> > vd->id = g_strdup(id);
> > - QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
> > + vd->dcl.ops = &dcl_ops;
> >
> > QTAILQ_INIT(&vd->clients);
> > vd->expires = TIME_MAX;
> > @@ -3445,22 +3448,22 @@ void vnc_display_init(const char *id, Error **errp)
> > }
> >
> > if (!vd->kbd_layout) {
> > + vnc_display_free(vd);
> > return;
> > }
> >
> > vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> > vd->connections_limit = 32;
> >
> > - qemu_mutex_init(&vd->mutex);
> > vnc_start_worker_thread();
> >
> > - vd->dcl.ops = &dcl_ops;
> > register_displaychangelistener(&vd->dcl);
> > vd->kbd = qkbd_state_init(vd->dcl.con);
> > vd->vmstate_handler_entry = qemu_add_vm_change_state_handler(
> > &vmstate_change_handler, vd);
> > -}
> >
> > + QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
> > +}
> >
> > static void vnc_display_close(VncDisplay *vd)
> > {
> > @@ -3504,6 +3507,25 @@ static void vnc_display_close(VncDisplay *vd)
> > #endif
> > }
> >
> > +static void vnc_display_free(VncDisplay *vd)
> > +{
> > + if (!vd) {
> > + return;
> > + }
> > + vnc_display_close(vd);
> > + unregister_displaychangelistener(&vd->dcl);
> > + qkbd_state_free(vd->kbd);
> > + qemu_del_vm_change_state_handler(vd->vmstate_handler_entry);
> > + kbd_layout_free(vd->kbd_layout);
> > + qemu_mutex_destroy(&vd->mutex);
> > + if (QTAILQ_IN_USE(vd, next)) {
> > + QTAILQ_REMOVE(&vnc_displays, vd, next);
> > + }
> > + g_free(vd->id);
> > + g_free(vd);
> > +}
>
> If we're introducing this we need to answer the earlier questions
> in this series about killing off the VNC worker thread, as IMHO,
> we should not leave the thread running if we're claiming to be
> able to free VncDisplay state.
Well, the worker thread is not directly associated with a VncDisplay.
Various VncState (VNC clients) share it. We could delay starting it
until jobs are actually submitted.
It is the caller's responsibility to ensure the VncDisplay has no
pending clients (and thus jobs) before calling vnc_display_free().
I'll add an assert() precondition.
>
> > +
> > +
> > int vnc_display_password(const char *id, const char *password, Error **errp)
> > {
> > VncDisplay *vd = vnc_display_find(id);
> >
> > --
> > 2.53.0
> >
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com ~~ https://hachyderm.io/@berrange :|
> |: https://libvirt.org ~~ https://entangle-photo.org :|
> |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
>