Hi On Mon, Mar 18, 2024 at 1:05 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote: > > On 2024/03/18 17:21, Marc-André Lureau wrote: > > Hi > > > > On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki <akihiko.od...@daynix.com> > > wrote: > >> > >> console_select() is shared by other displays and a console_select() call > >> from one of them triggers console switching also in ui/curses, > >> circumventing key state reinitialization that needs to be performed in > >> preparation and resulting in stuck keys. > >> > >> Use its internal state to track the current active console to prevent > >> such a surprise console switch. > >> > >> Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com> > >> --- > >> include/ui/console.h | 1 + > >> include/ui/kbd-state.h | 11 +++++++++++ > >> ui/console.c | 12 ++++++++++++ > >> ui/kbd-state.c | 6 ++++++ > >> ui/vnc.c | 14 +++++++++----- > >> 5 files changed, 39 insertions(+), 5 deletions(-) > >> > >> diff --git a/include/ui/console.h b/include/ui/console.h > >> index a4a49ffc640c..a703f7466499 100644 > >> --- a/include/ui/console.h > >> +++ b/include/ui/console.h > >> @@ -413,6 +413,7 @@ void qemu_console_early_init(void); > >> > >> void qemu_console_set_display_gl_ctx(QemuConsole *con, DisplayGLCtx > >> *ctx); > >> > >> +QemuConsole *qemu_console_lookup_first_graphic_console(void); > >> QemuConsole *qemu_console_lookup_by_index(unsigned int index); > >> QemuConsole *qemu_console_lookup_by_device(DeviceState *dev, uint32_t > >> head); > >> QemuConsole *qemu_console_lookup_by_device_name(const char *device_id, > >> diff --git a/include/ui/kbd-state.h b/include/ui/kbd-state.h > >> index fb79776128cf..1f37b932eb62 100644 > >> --- a/include/ui/kbd-state.h > >> +++ b/include/ui/kbd-state.h > >> @@ -99,4 +99,15 @@ bool qkbd_state_modifier_get(QKbdState *kbd, > >> QKbdModifier mod); > >> */ > >> void qkbd_state_lift_all_keys(QKbdState *kbd); > >> > >> +/** > >> + * qkbd_state_switch_console: Switch console. > >> + * > >> + * This sends key up events to the previous console for all keys which > >> are in > >> + * down state to prevent keys being stuck, and remembers the new console. > >> + * > >> + * @kbd: state tracker state. > >> + * @con: new QemuConsole for this state tracker. > >> + */ > >> +void qkbd_state_switch_console(QKbdState *kbd, QemuConsole *con); > >> + > >> #endif /* QEMU_UI_KBD_STATE_H */ > >> diff --git a/ui/console.c b/ui/console.c > >> index 832055675c50..6bf02a23414c 100644 > >> --- a/ui/console.c > >> +++ b/ui/console.c > >> @@ -1325,6 +1325,18 @@ void graphic_console_close(QemuConsole *con) > >> dpy_gfx_replace_surface(con, surface); > >> } > >> > >> +QemuConsole *qemu_console_lookup_first_graphic_console(void) > >> +{ > >> + QemuConsole *con; > >> + > >> + QTAILQ_FOREACH(con, &consoles, next) { > >> + if (QEMU_IS_GRAPHIC_CONSOLE(con)) { > >> + return con; > >> + } > >> + } > >> + return NULL; > >> +} > >> + > >> QemuConsole *qemu_console_lookup_by_index(unsigned int index) > >> { > >> QemuConsole *con; > >> diff --git a/ui/kbd-state.c b/ui/kbd-state.c > >> index 62d42a7a22e1..52ed28b8a89b 100644 > >> --- a/ui/kbd-state.c > >> +++ b/ui/kbd-state.c > >> @@ -117,6 +117,12 @@ void qkbd_state_lift_all_keys(QKbdState *kbd) > >> } > >> } > >> > >> +void qkbd_state_switch_console(QKbdState *kbd, QemuConsole *con) > >> +{ > >> + qkbd_state_lift_all_keys(kbd); > >> + kbd->con = con; > >> +} > >> + > >> void qkbd_state_set_delay(QKbdState *kbd, int delay_ms) > >> { > >> kbd->key_delay_ms = delay_ms; > >> diff --git a/ui/vnc.c b/ui/vnc.c > >> index fc12b343e254..94564b196ba8 100644 > >> --- a/ui/vnc.c > >> +++ b/ui/vnc.c > >> @@ -1872,12 +1872,16 @@ static void do_key_event(VncState *vs, int down, > >> int keycode, int sym) > >> /* QEMU console switch */ > >> switch (qcode) { > >> case Q_KEY_CODE_1 ... Q_KEY_CODE_9: /* '1' to '9' keys */ > >> - if (vs->vd->dcl.con == NULL && down && > >> + if (down && > >> qkbd_state_modifier_get(vs->vd->kbd, QKBD_MOD_CTRL) && > >> qkbd_state_modifier_get(vs->vd->kbd, QKBD_MOD_ALT)) { > >> - /* Reset the modifiers sent to the current console */ > >> - qkbd_state_lift_all_keys(vs->vd->kbd); > >> - console_select(qcode - Q_KEY_CODE_1); > >> + QemuConsole *con = qemu_console_lookup_by_index(qcode - > >> Q_KEY_CODE_1); > >> + if (con) { > >> + unregister_displaychangelistener(&vs->vd->dcl); > >> + qkbd_state_switch_console(vs->vd->kbd, con); > >> + vs->vd->dcl.con = con; > >> + register_displaychangelistener(&vs->vd->dcl); > >> + } > >> return; > >> } > >> default: > >> @@ -4206,7 +4210,7 @@ void vnc_display_open(const char *id, Error **errp) > >> goto fail; > >> } > >> } else { > >> - con = NULL; > >> + con = qemu_console_lookup_first_graphic_console(); > > > > why this change here? > > console_select() is to change the console that is used when > DisplayChangeListener::con is NULL. console_select() is no longer called > so DisplayChangeListener::con must not be NULL.
But qemu_console_lookup_first_graphic_console() can return NULL. It's problematic for the next patches also which seem to assume that NULL console is no longer a valid argument. We would need a lot of assert(con != NULL) or similar to ensure this holds. > > > > otherwise, lgtm > > > >> } > >> > >> if (con != vd->dcl.con) { > >> > >> -- > >> 2.44.0 > >> > >> > > > > -- Marc-André Lureau