| Message ID | 20240318-console-v1-2-f4efbfa71253@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | ui/console: Remove console_select() | expand |
Hi On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki <akihiko.odaki@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.odaki@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? otherwise, lgtm > } > > if (con != vd->dcl.con) { > > -- > 2.44.0 > >
On 2024/03/18 17:21, Marc-André Lureau wrote: > Hi > > On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki <akihiko.odaki@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.odaki@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. > > otherwise, lgtm > >> } >> >> if (con != vd->dcl.con) { >> >> -- >> 2.44.0 >> >> > >
Hi On Mon, Mar 18, 2024 at 1:05 PM Akihiko Odaki <akihiko.odaki@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.odaki@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.odaki@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 > >> > >> > > > >
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(); } if (con != vd->dcl.con) {
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.odaki@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(-)