Re: [PATCH 2/4] ui/vnc: Do not use console_select()
Hi
On Mon, Mar 18, 2024 at 1:05 PM Akihiko Odaki wrote:
>
> On 2024/03/18 17:21, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki
> > 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
> >> ---
> >> 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, , 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(>vd->dcl);
> >> +qkbd_state_switch_console(vs->vd->kbd, con);
> >> +vs->vd->dcl.con = con;
> >> +register_displaychangelistener(>vd->dcl);
> >> +}
> >> return;
> >> }
> >> default:
> >> @@ -4206,7 +4210,7 @@ void vnc_display_open(const char *id, Error **errp)
> >> goto fail;
> >> }
> >> } else {
> >> -con = NULL;
> >> +con =
Re: [PATCH 2/4] ui/vnc: Do not use console_select()
On 2024/03/18 17:21, Marc-André Lureau wrote:
Hi
On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki 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
---
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, , 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(>vd->dcl);
+qkbd_state_switch_console(vs->vd->kbd, con);
+vs->vd->dcl.con = con;
+register_displaychangelistener(>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
Re: [PATCH 2/4] ui/vnc: Do not use console_select()
Hi
On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki 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
> ---
> 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, , 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(>vd->dcl);
> +qkbd_state_switch_console(vs->vd->kbd, con);
> +vs->vd->dcl.con = con;
> +register_displaychangelistener(>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
>
>
--
Marc-André Lureau
[PATCH 2/4] ui/vnc: Do not use console_select()
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
---
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, , 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(>vd->dcl);
+qkbd_state_switch_console(vs->vd->kbd, con);
+vs->vd->dcl.con = con;
+register_displaychangelistener(>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) {
--
2.44.0
