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

[Marc-André Lureau]

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



-- 
Marc-André Lureau