Re: [PATCH 2/4] ui/vnc: Do not use console_select()

[Marc-André Lureau]

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
>
>


-- 
Marc-André Lureau