[PATCH 0/4] ui/console: Remove console_select()

[PATCH 0/4] ui/console: Remove console_select()

[Akihiko Odaki]

ui/console has a concept of "active" console; the active console is used
when NULL is set for DisplayListener::con, and console_select() updates
the active console state. However, the global nature of the state cause
odd behaviors, and replacing NULL with the active console also resulted
in extra code. Remove it to solve these problems.

The active console state is shared, so if there are two displays
referring to the active console, switching the console for one will also
affect the other. All displays that use the active console state,
namely cocoa, curses, and vnc, need to reset some of its state before
switching the console, and such a reset operation cannot be performed if
the console is switched by another display. This can result in stuck
keys, for example.

While the active console state is shared, displays other than cocoa,
curses, and vnc don't update the state. A chardev-vc inherits the
size of the active console, but it does not make sense for such a
display.

This series removes the shared "active" console state from ui/console.
curses, cocoa, and vnc will hold the reference to the console currently
shown with DisplayListener::con. This also eliminates the need to
replace NULL with the active console and save code.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (4):
      ui/vc: Do not inherit the size of active console
      ui/vnc: Do not use console_select()
      ui/cocoa: Do not use console_select()
      ui/curses: Do not use console_select()

 include/ui/console.h   |   2 +-
 include/ui/kbd-state.h |  11 ++++
 ui/console-priv.h      |   2 +-
 ui/console-vc-stubs.c  |   2 +-
 ui/console-vc.c        |   7 ++-
 ui/console.c           | 133 ++++++++++++-------------------------------------
 ui/curses.c            |  48 ++++++++++--------
 ui/kbd-state.c         |   6 +++
 ui/vnc.c               |  14 ++++--
 ui/cocoa.m             |  37 ++++++++++----
 10 files changed, 118 insertions(+), 144 deletions(-)
---
base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b
change-id: 20240317-console-6744d4ab8ba6

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH 1/4] ui/vc: Do not inherit the size of active console

[Akihiko Odaki]

A chardev-vc used to inherit the size of a graphic console when its
size not explicitly specified, but it often did not make sense. If a
chardev-vc is instantiated during the startup, the active graphic
console has no content at the time, so it will have the size of graphic
console placeholder, which contains no useful information. It's better
to have the standard size of text console instead.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 ui/console-vc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/console-vc.c b/ui/console-vc.c
index 9c13cc2981b0..f22c8e23c2ed 100644
--- a/ui/console-vc.c
+++ b/ui/console-vc.c
@@ -990,8 +990,8 @@ static void vc_chr_open(Chardev *chr,
     trace_console_txt_new(width, height);
     if (width == 0 || height == 0) {
         s = QEMU_TEXT_CONSOLE(object_new(TYPE_QEMU_TEXT_CONSOLE));
-        width = qemu_console_get_width(NULL, 80 * FONT_WIDTH);
-        height = qemu_console_get_height(NULL, 24 * FONT_HEIGHT);
+        width = 80 * FONT_WIDTH;
+        height = 24 * FONT_HEIGHT;
     } else {
         s = QEMU_TEXT_CONSOLE(object_new(TYPE_QEMU_FIXED_TEXT_CONSOLE));
     }

-- 
2.44.0

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

[Akihiko Odaki]

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();
     }
 
     if (con != vd->dcl.con) {

-- 
2.44.0

[PATCH 3/4] ui/cocoa: Do not use console_select()

[Akihiko Odaki]

ui/cocoa needs to update the UI info and reset the keyboard state
tracker when switching the console, or the new console will see the
stale UI info or keyboard state. Previously, updating the UI info was
done with cocoa_switch(), but it is meant to be called when the surface
is being replaced, and may be called even when not switching the
console. ui/cocoa never reset the keyboard state, which resulted in
stuck keys.

Add ui/cocoa's own implementation of console_select(), which updates the
UI info and resets the keyboard state tracker.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 ui/cocoa.m | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index fa879d7dcd4b..47280c0a93be 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -102,6 +102,7 @@ static void cocoa_switch(DisplayChangeListener *dcl,
 static DisplayChangeListener dcl = {
     .ops = &dcl_ops,
 };
+static QKbdState *kbd;
 static int cursor_hide = 1;
 static int left_command_key_enabled = 1;
 static bool swap_opt_cmd;
@@ -309,7 +310,6 @@ @interface QemuCocoaView : NSView
     NSTrackingArea *trackingArea;
     QEMUScreen screen;
     pixman_image_t *pixman_image;
-    QKbdState *kbd;
     BOOL isMouseGrabbed;
     BOOL isAbsoluteEnabled;
     CFMachPortRef eventsTap;
@@ -361,7 +361,6 @@ - (id)initWithFrame:(NSRect)frameRect
 
         screen.width = frameRect.size.width;
         screen.height = frameRect.size.height;
-        kbd = qkbd_state_init(dcl.con);
 #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_14_0
         [self setClipsToBounds:YES];
 #endif
@@ -378,8 +377,6 @@ - (void) dealloc
         pixman_image_unref(pixman_image);
     }
 
-    qkbd_state_free(kbd);
-
     if (eventsTap) {
         CFRelease(eventsTap);
     }
@@ -429,6 +426,20 @@ - (void) viewWillMoveToWindow:(NSWindow *)newWindow
     [self removeTrackingRect];
 }
 
+- (void) selectConsoleLocked:(unsigned int)index
+{
+    QemuConsole *con = qemu_console_lookup_by_index(index);
+    if (!con) {
+        return;
+    }
+
+    unregister_displaychangelistener(&dcl);
+    qkbd_state_switch_console(kbd, con);
+    dcl.con = con;
+    register_displaychangelistener(&dcl);
+    [self updateUIInfo];
+}
+
 - (void) hideCursor
 {
     if (!cursor_hide) {
@@ -718,7 +729,8 @@ - (void) handleMonitorInput:(NSEvent *)event
     }
 
     if (keysym) {
-        qemu_text_console_put_keysym(NULL, keysym);
+        QemuTextConsole *con = QEMU_TEXT_CONSOLE(dcl.con);
+        qemu_text_console_put_keysym(con, keysym);
     }
 }
 
@@ -898,7 +910,7 @@ - (bool) handleEventLocked:(NSEvent *)event
 
                         // enable graphic console
                         case '1' ... '9':
-                            console_select(key - '0' - 1); /* ascii math */
+                            [self selectConsoleLocked:key - '0' - 1]; /* ascii math */
                             return true;
 
                         // release the mouse grab
@@ -909,7 +921,7 @@ - (bool) handleEventLocked:(NSEvent *)event
                 }
             }
 
-            if (qemu_console_is_graphic(NULL)) {
+            if (qemu_console_is_graphic(dcl.con)) {
                 qkbd_state_key_event(kbd, keycode, true);
             } else {
                 [self handleMonitorInput: event];
@@ -924,7 +936,7 @@ - (bool) handleEventLocked:(NSEvent *)event
                 return true;
             }
 
-            if (qemu_console_is_graphic(NULL)) {
+            if (qemu_console_is_graphic(dcl.con)) {
                 qkbd_state_key_event(kbd, keycode, false);
             }
             return true;
@@ -1374,7 +1386,7 @@ - (void)toggleZoomInterpolation:(id) sender
 - (void)displayConsole:(id)sender
 {
     with_bql(^{
-        console_select([sender tag]);
+        [cocoaView selectConsoleLocked:[sender tag]];
     });
 }
 
@@ -1945,7 +1957,6 @@ static void cocoa_switch(DisplayChangeListener *dcl,
     pixman_image_ref(image);
 
     dispatch_async(dispatch_get_main_queue(), ^{
-        [cocoaView updateUIInfo];
         [cocoaView switchSurface:image];
     });
 }
@@ -1955,7 +1966,7 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
     NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
 
     COCOA_DEBUG("qemu_cocoa: cocoa_refresh\n");
-    graphic_hw_update(NULL);
+    graphic_hw_update(dcl->con);
 
     if (qemu_input_is_absolute(dcl->con)) {
         dispatch_async(dispatch_get_main_queue(), ^{
@@ -2039,8 +2050,12 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
     add_console_menu_entries();
     addRemovableDevicesMenuItems();
 
+    dcl.con = qemu_console_lookup_first_graphic_console();
+    kbd = qkbd_state_init(dcl.con);
+
     // register vga output callbacks
     register_displaychangelistener(&dcl);
+    [cocoaView updateUIInfo];
 
     qemu_event_init(&cbevent, false);
     cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];

-- 
2.44.0

[PATCH 4/4] ui/curses: Do not use console_select()

[Akihiko Odaki]

ui/curses is the only user of console_select(). Move the implementation
to ui/curses.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/ui/console.h  |   1 -
 ui/console-priv.h     |   2 +-
 ui/console-vc-stubs.c |   2 +-
 ui/console-vc.c       |   3 +-
 ui/console.c          | 121 +++++++++-----------------------------------------
 ui/curses.c           |  48 +++++++++++---------
 6 files changed, 51 insertions(+), 126 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index a703f7466499..3769179e3b2b 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -433,7 +433,6 @@ int qemu_console_get_window_id(QemuConsole *con);
 /* Set the low-level window id for the console */
 void qemu_console_set_window_id(QemuConsole *con, int window_id);
 
-void console_select(unsigned int index);
 void qemu_console_resize(QemuConsole *con, int width, int height);
 DisplaySurface *qemu_console_surface(QemuConsole *con);
 void coroutine_fn qemu_console_co_wait_update(QemuConsole *con);
diff --git a/ui/console-priv.h b/ui/console-priv.h
index 88569ed2cc41..43ceb8122f13 100644
--- a/ui/console-priv.h
+++ b/ui/console-priv.h
@@ -35,7 +35,7 @@ struct QemuConsole {
     QTAILQ_ENTRY(QemuConsole) next;
 };
 
-void qemu_text_console_select(QemuTextConsole *c);
+void qemu_text_console_update_size(QemuTextConsole *c);
 const char * qemu_text_console_get_label(QemuTextConsole *c);
 void qemu_text_console_update_cursor(void);
 void qemu_text_console_handle_keysym(QemuTextConsole *s, int keysym);
diff --git a/ui/console-vc-stubs.c b/ui/console-vc-stubs.c
index 2afc52329f0c..b63e2fb2345f 100644
--- a/ui/console-vc-stubs.c
+++ b/ui/console-vc-stubs.c
@@ -10,7 +10,7 @@
 #include "chardev/char.h"
 #include "ui/console-priv.h"
 
-void qemu_text_console_select(QemuTextConsole *c)
+void qemu_text_console_update_size(QemuTextConsole *c)
 {
 }
 
diff --git a/ui/console-vc.c b/ui/console-vc.c
index f22c8e23c2ed..899fa11c9485 100644
--- a/ui/console-vc.c
+++ b/ui/console-vc.c
@@ -958,10 +958,9 @@ static void vc_chr_set_echo(Chardev *chr, bool echo)
     drv->console->echo = echo;
 }
 
-void qemu_text_console_select(QemuTextConsole *c)
+void qemu_text_console_update_size(QemuTextConsole *c)
 {
     dpy_text_resize(QEMU_CONSOLE(c), c->width, c->height);
-    qemu_text_console_update_cursor();
 }
 
 static void vc_chr_open(Chardev *chr,
diff --git a/ui/console.c b/ui/console.c
index 6bf02a23414c..d77f509046af 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -66,7 +66,6 @@ struct DisplayState {
 };
 
 static DisplayState *display_state;
-static QemuConsole *active_console;
 static QTAILQ_HEAD(, QemuConsole) consoles =
     QTAILQ_HEAD_INITIALIZER(consoles);
 
@@ -135,7 +134,6 @@ void graphic_hw_update_done(QemuConsole *con)
 void graphic_hw_update(QemuConsole *con)
 {
     bool async = false;
-    con = con ? con : active_console;
     if (!con) {
         return;
     }
@@ -209,9 +207,6 @@ void qemu_console_set_window_id(QemuConsole *con, int window_id)
 
 void graphic_hw_invalidate(QemuConsole *con)
 {
-    if (!con) {
-        con = active_console;
-    }
     if (con && con->hw_ops->invalidate) {
         con->hw_ops->invalidate(con->hw);
     }
@@ -219,9 +214,6 @@ void graphic_hw_invalidate(QemuConsole *con)
 
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
 {
-    if (!con) {
-        con = active_console;
-    }
     if (con && con->hw_ops->text_update) {
         con->hw_ops->text_update(con->hw, chardata);
     }
@@ -265,12 +257,12 @@ static void dpy_gfx_update_texture(QemuConsole *con, DisplaySurface *surface,
 }
 
 static void displaychangelistener_display_console(DisplayChangeListener *dcl,
-                                                  QemuConsole *con,
                                                   Error **errp)
 {
     static const char nodev[] =
         "This VM has no graphic display device.";
     static DisplaySurface *dummy;
+    QemuConsole *con = dcl->con;
 
     if (!con || !console_compatible_with(con, dcl, errp)) {
         if (!dummy) {
@@ -305,39 +297,8 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
     }
 }
 
-void console_select(unsigned int index)
-{
-    DisplayChangeListener *dcl;
-    QemuConsole *s;
-
-    trace_console_select(index);
-    s = qemu_console_lookup_by_index(index);
-    if (s) {
-        DisplayState *ds = s->ds;
-
-        active_console = s;
-        QLIST_FOREACH (dcl, &ds->listeners, next) {
-            if (dcl->con != NULL) {
-                continue;
-            }
-            displaychangelistener_display_console(dcl, s, NULL);
-        }
-
-        if (QEMU_IS_TEXT_CONSOLE(s)) {
-            qemu_text_console_select(QEMU_TEXT_CONSOLE(s));
-        }
-    }
-}
-
 void qemu_text_console_put_keysym(QemuTextConsole *s, int keysym)
 {
-    if (!s) {
-        if (!QEMU_IS_TEXT_CONSOLE(active_console)) {
-            return;
-        }
-        s = QEMU_TEXT_CONSOLE(active_console);
-    }
-
     qemu_text_console_handle_keysym(s, keysym);
 }
 
@@ -392,11 +353,6 @@ qemu_console_register(QemuConsole *c)
 {
     int i;
 
-    if (!active_console || (!QEMU_IS_GRAPHIC_CONSOLE(active_console) &&
-                            QEMU_IS_GRAPHIC_CONSOLE(c))) {
-        active_console = c;
-    }
-
     if (QTAILQ_EMPTY(&consoles)) {
         c->index = 0;
         QTAILQ_INSERT_TAIL(&consoles, c, next);
@@ -751,8 +707,6 @@ dcl_set_graphic_cursor(DisplayChangeListener *dcl, QemuGraphicConsole *con)
 
 void register_displaychangelistener(DisplayChangeListener *dcl)
 {
-    QemuConsole *con;
-
     assert(!dcl->ds);
 
     trace_displaychangelistener_register(dcl, dcl->ops->dpy_name);
@@ -761,13 +715,12 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
     gui_setup_refresh(dcl->ds);
     if (dcl->con) {
         dcl->con->dcls++;
-        con = dcl->con;
-    } else {
-        con = active_console;
     }
-    displaychangelistener_display_console(dcl, con, dcl->con ? &error_fatal : NULL);
-    if (QEMU_IS_GRAPHIC_CONSOLE(con)) {
-        dcl_set_graphic_cursor(dcl, QEMU_GRAPHIC_CONSOLE(con));
+    displaychangelistener_display_console(dcl, &error_fatal);
+    if (QEMU_IS_GRAPHIC_CONSOLE(dcl->con)) {
+        dcl_set_graphic_cursor(dcl, QEMU_GRAPHIC_CONSOLE(dcl->con));
+    } else if (QEMU_IS_TEXT_CONSOLE(dcl->con)) {
+        qemu_text_console_update_size(QEMU_TEXT_CONSOLE(dcl->con));
     }
     qemu_text_console_update_cursor();
 }
@@ -805,9 +758,6 @@ static void dpy_set_ui_info_timer(void *opaque)
 
 bool dpy_ui_info_supported(const QemuConsole *con)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
     if (con == NULL) {
         return false;
     }
@@ -819,19 +769,11 @@ const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
 {
     assert(dpy_ui_info_supported(con));
 
-    if (con == NULL) {
-        con = active_console;
-    }
-
     return &con->ui_info;
 }
 
 int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info, bool delay)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
-
     if (!dpy_ui_info_supported(con)) {
         return -1;
     }
@@ -870,7 +812,7 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h)
     }
     dpy_gfx_update_texture(con, con->surface, x, y, w, h);
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_gfx_update) {
@@ -916,7 +858,7 @@ void dpy_gfx_replace_surface(QemuConsole *con,
     con->surface = new_surface;
     dpy_gfx_create_texture(con, new_surface);
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         displaychangelistener_gfx_switch(dcl, new_surface, surface ? FALSE : TRUE);
@@ -970,7 +912,7 @@ void dpy_text_cursor(QemuConsole *con, int x, int y)
         return;
     }
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_text_cursor) {
@@ -988,7 +930,7 @@ void dpy_text_update(QemuConsole *con, int x, int y, int w, int h)
         return;
     }
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_text_update) {
@@ -1006,7 +948,7 @@ void dpy_text_resize(QemuConsole *con, int w, int h)
         return;
     }
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_text_resize) {
@@ -1028,7 +970,7 @@ void dpy_mouse_set(QemuConsole *c, int x, int y, int on)
         return;
     }
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (c != (dcl->con ? dcl->con : active_console)) {
+        if (c != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_mouse_set) {
@@ -1049,7 +991,7 @@ void dpy_cursor_define(QemuConsole *c, QEMUCursor *cursor)
         return;
     }
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (c != (dcl->con ? dcl->con : active_console)) {
+        if (c != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_cursor_define) {
@@ -1099,7 +1041,7 @@ void dpy_gl_scanout_disable(QemuConsole *con)
         con->scanout.kind = SCANOUT_NONE;
     }
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_gl_scanout_disable) {
@@ -1126,7 +1068,7 @@ void dpy_gl_scanout_texture(QemuConsole *con,
         x, y, width, height, d3d_tex2d,
     };
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_gl_scanout_texture) {
@@ -1148,7 +1090,7 @@ void dpy_gl_scanout_dmabuf(QemuConsole *con,
     con->scanout.kind = SCANOUT_DMABUF;
     con->scanout.dmabuf = dmabuf;
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_gl_scanout_dmabuf) {
@@ -1164,7 +1106,7 @@ void dpy_gl_cursor_dmabuf(QemuConsole *con, QemuDmaBuf *dmabuf,
     DisplayChangeListener *dcl;
 
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_gl_cursor_dmabuf) {
@@ -1181,7 +1123,7 @@ void dpy_gl_cursor_position(QemuConsole *con,
     DisplayChangeListener *dcl;
 
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_gl_cursor_position) {
@@ -1197,7 +1139,7 @@ void dpy_gl_release_dmabuf(QemuConsole *con,
     DisplayChangeListener *dcl;
 
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_gl_release_dmabuf) {
@@ -1216,7 +1158,7 @@ void dpy_gl_update(QemuConsole *con,
 
     graphic_hw_gl_block(con, true);
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (con != (dcl->con ? dcl->con : active_console)) {
+        if (con != dcl->con) {
             continue;
         }
         if (dcl->ops->dpy_gl_update) {
@@ -1415,30 +1357,21 @@ static QemuConsole *qemu_graphic_console_lookup_unused(void)
 
 QEMUCursor *qemu_console_get_cursor(QemuConsole *con)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
     return QEMU_IS_GRAPHIC_CONSOLE(con) ? QEMU_GRAPHIC_CONSOLE(con)->cursor : NULL;
 }
 
 bool qemu_console_is_visible(QemuConsole *con)
 {
-    return (con == active_console) || (con->dcls > 0);
+    return con->dcls > 0;
 }
 
 bool qemu_console_is_graphic(QemuConsole *con)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
     return con && QEMU_IS_GRAPHIC_CONSOLE(con);
 }
 
 bool qemu_console_is_fixedsize(QemuConsole *con)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
     return con && (QEMU_IS_GRAPHIC_CONSOLE(con) || QEMU_IS_FIXED_TEXT_CONSOLE(con));
 }
 
@@ -1505,17 +1438,11 @@ char *qemu_console_get_label(QemuConsole *con)
 
 int qemu_console_get_index(QemuConsole *con)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
     return con ? con->index : -1;
 }
 
 uint32_t qemu_console_get_head(QemuConsole *con)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
     if (con == NULL) {
         return -1;
     }
@@ -1527,9 +1454,6 @@ uint32_t qemu_console_get_head(QemuConsole *con)
 
 int qemu_console_get_width(QemuConsole *con, int fallback)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
     if (con == NULL) {
         return fallback;
     }
@@ -1547,9 +1471,6 @@ int qemu_console_get_width(QemuConsole *con, int fallback)
 
 int qemu_console_get_height(QemuConsole *con, int fallback)
 {
-    if (con == NULL) {
-        con = active_console;
-    }
     if (con == NULL) {
         return fallback;
     }
diff --git a/ui/curses.c b/ui/curses.c
index 8bde8c5cf7c3..df57c322d384 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -98,7 +98,7 @@ static void curses_update(DisplayChangeListener *dcl,
 
 static void curses_calc_pad(void)
 {
-    if (qemu_console_is_fixedsize(NULL)) {
+    if (qemu_console_is_fixedsize(dcl->con)) {
         width = gwidth;
         height = gheight;
     } else {
@@ -201,7 +201,7 @@ static void curses_cursor_position(DisplayChangeListener *dcl,
             curs_set(1);
             /* it seems that curs_set(1) must always be called before
              * curs_set(2) for the latter to have effect */
-            if (!qemu_console_is_graphic(NULL)) {
+            if (!qemu_console_is_graphic(dcl->con)) {
                 curs_set(2);
             }
             return;
@@ -274,11 +274,11 @@ static void curses_refresh(DisplayChangeListener *dcl)
         clear();
         refresh();
         curses_calc_pad();
-        graphic_hw_invalidate(NULL);
+        graphic_hw_invalidate(dcl->con);
         invalidate = 0;
     }
 
-    graphic_hw_text_update(NULL, screen);
+    graphic_hw_text_update(dcl->con, screen);
 
     while (1) {
         /* while there are any pending key strokes to process */
@@ -318,11 +318,16 @@ static void curses_refresh(DisplayChangeListener *dcl)
                     /* process keys reserved for qemu */
                     if (keycode >= QEMU_KEY_CONSOLE0 &&
                             keycode < QEMU_KEY_CONSOLE0 + 9) {
-                        erase();
-                        wnoutrefresh(stdscr);
-                        console_select(keycode - QEMU_KEY_CONSOLE0);
-
-                        invalidate = 1;
+                        QemuConsole *con = qemu_console_lookup_by_index(keycode - QEMU_KEY_CONSOLE0);
+                        if (con) {
+                            erase();
+                            wnoutrefresh(stdscr);
+                            unregister_displaychangelistener(dcl);
+                            dcl->con = con;
+                            register_displaychangelistener(dcl);
+
+                            invalidate = 1;
+                        }
                         continue;
                     }
                 }
@@ -354,45 +359,45 @@ static void curses_refresh(DisplayChangeListener *dcl)
         if (keycode == -1)
             continue;
 
-        if (qemu_console_is_graphic(NULL)) {
+        if (qemu_console_is_graphic(dcl->con)) {
             /* since terminals don't know about key press and release
              * events, we need to emit both for each key received */
             if (keycode & SHIFT) {
-                qemu_input_event_send_key_number(NULL, SHIFT_CODE, true);
+                qemu_input_event_send_key_number(dcl->con, SHIFT_CODE, true);
                 qemu_input_event_send_key_delay(0);
             }
             if (keycode & CNTRL) {
-                qemu_input_event_send_key_number(NULL, CNTRL_CODE, true);
+                qemu_input_event_send_key_number(dcl->con, CNTRL_CODE, true);
                 qemu_input_event_send_key_delay(0);
             }
             if (keycode & ALT) {
-                qemu_input_event_send_key_number(NULL, ALT_CODE, true);
+                qemu_input_event_send_key_number(dcl->con, ALT_CODE, true);
                 qemu_input_event_send_key_delay(0);
             }
             if (keycode & ALTGR) {
-                qemu_input_event_send_key_number(NULL, GREY | ALT_CODE, true);
+                qemu_input_event_send_key_number(dcl->con, GREY | ALT_CODE, true);
                 qemu_input_event_send_key_delay(0);
             }
 
-            qemu_input_event_send_key_number(NULL, keycode & KEY_MASK, true);
+            qemu_input_event_send_key_number(dcl->con, keycode & KEY_MASK, true);
             qemu_input_event_send_key_delay(0);
-            qemu_input_event_send_key_number(NULL, keycode & KEY_MASK, false);
+            qemu_input_event_send_key_number(dcl->con, keycode & KEY_MASK, false);
             qemu_input_event_send_key_delay(0);
 
             if (keycode & ALTGR) {
-                qemu_input_event_send_key_number(NULL, GREY | ALT_CODE, false);
+                qemu_input_event_send_key_number(dcl->con, GREY | ALT_CODE, false);
                 qemu_input_event_send_key_delay(0);
             }
             if (keycode & ALT) {
-                qemu_input_event_send_key_number(NULL, ALT_CODE, false);
+                qemu_input_event_send_key_number(dcl->con, ALT_CODE, false);
                 qemu_input_event_send_key_delay(0);
             }
             if (keycode & CNTRL) {
-                qemu_input_event_send_key_number(NULL, CNTRL_CODE, false);
+                qemu_input_event_send_key_number(dcl->con, CNTRL_CODE, false);
                 qemu_input_event_send_key_delay(0);
             }
             if (keycode & SHIFT) {
-                qemu_input_event_send_key_number(NULL, SHIFT_CODE, false);
+                qemu_input_event_send_key_number(dcl->con, SHIFT_CODE, false);
                 qemu_input_event_send_key_delay(0);
             }
         } else {
@@ -400,7 +405,7 @@ static void curses_refresh(DisplayChangeListener *dcl)
             if (keysym == -1)
                 keysym = chr;
 
-            qemu_text_console_put_keysym(NULL, keysym);
+            qemu_text_console_put_keysym(QEMU_TEXT_CONSOLE(dcl->con), keysym);
         }
     }
 }
@@ -798,6 +803,7 @@ static void curses_display_init(DisplayState *ds, DisplayOptions *opts)
     curses_winch_init();
 
     dcl = g_new0(DisplayChangeListener, 1);
+    dcl->con = qemu_console_lookup_first_graphic_console();
     dcl->ops = &dcl_ops;
     register_displaychangelistener(dcl);
 

-- 
2.44.0

Re: [PATCH 1/4] ui/vc: Do not inherit the size of active console

[Marc-André Lureau]

On Mon, Mar 18, 2024 at 11:59 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> A chardev-vc used to inherit the size of a graphic console when its
> size not explicitly specified, but it often did not make sense. If a
> chardev-vc is instantiated during the startup, the active graphic
> console has no content at the time, so it will have the size of graphic
> console placeholder, which contains no useful information. It's better
> to have the standard size of text console instead.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  ui/console-vc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ui/console-vc.c b/ui/console-vc.c
> index 9c13cc2981b0..f22c8e23c2ed 100644
> --- a/ui/console-vc.c
> +++ b/ui/console-vc.c
> @@ -990,8 +990,8 @@ static void vc_chr_open(Chardev *chr,
>      trace_console_txt_new(width, height);
>      if (width == 0 || height == 0) {
>          s = QEMU_TEXT_CONSOLE(object_new(TYPE_QEMU_TEXT_CONSOLE));
> -        width = qemu_console_get_width(NULL, 80 * FONT_WIDTH);
> -        height = qemu_console_get_height(NULL, 24 * FONT_HEIGHT);
> +        width = 80 * FONT_WIDTH;
> +        height = 24 * FONT_HEIGHT;
>      } else {
>          s = QEMU_TEXT_CONSOLE(object_new(TYPE_QEMU_FIXED_TEXT_CONSOLE));
>      }
>
> --
> 2.44.0
>
>


-- 
Marc-André Lureau

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

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

[Akihiko Odaki]

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

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