[Qemu-devel] [RFC PATCH v2 0/7] ui: add generic keyboard state tracker, fix keymap

[Qemu-devel] [RFC PATCH v2 0/7] ui: add generic keyboard state tracker, fix keymap

[Gerd Hoffmann]

Gerd Hoffmann (7):
  kbd-state: add keyboard state tracker
  kbd-state: use state tracker for sdl2
  sdl2: use only QKeyCode in sdl2_process_key()
  kbd-state: use state tracker for gtk
  kbd-state: use state tracker for vnc
  keymap: pass full keyboard state to keysym2scancode
  keymap: fix keyup mappings

 include/ui/gtk.h       |   2 +
 include/ui/kbd-state.h |  32 +++++++++++++
 include/ui/sdl2.h      |   2 +
 ui/keymaps.h           |   3 +-
 ui/vnc.h               |   5 +-
 ui/curses.c            |   2 +-
 ui/gtk.c               |  38 +++------------
 ui/kbd-state.c         | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
 ui/keymaps.c           |  55 +++++++++++++---------
 ui/sdl2-input.c        |  47 +++----------------
 ui/sdl2.c              |  12 ++---
 ui/vnc.c               | 119 ++++++++++++----------------------------------
 ui/Makefile.objs       |   2 +-
 13 files changed, 247 insertions(+), 197 deletions(-)
 create mode 100644 include/ui/kbd-state.h
 create mode 100644 ui/kbd-state.c

-- 
2.9.3

[Qemu-devel] [RFC PATCH v2 1/7] kbd-state: add keyboard state tracker

[Gerd Hoffmann]

Now that most user interfaces are using QKeyCodes it is easier to have
common keyboard code useable by all user interfaces.

This patch adds helper code to track the state of all keyboard keys,
using a bitmap indexed by QKeyCode.  Modifier state is tracked too,
as separate bitmap.  That makes checking modifier state easier.
Likewise we can easily apply special handling for capslock & numlock
(toggles on keypress) and ctrl + shift (we have two keys for that).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/kbd-state.h |  32 +++++++++++++
 ui/kbd-state.c         | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
 ui/Makefile.objs       |   2 +-
 3 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 include/ui/kbd-state.h
 create mode 100644 ui/kbd-state.c

diff --git a/include/ui/kbd-state.h b/include/ui/kbd-state.h
new file mode 100644
index 0000000000..0bef75a5d5
--- /dev/null
+++ b/include/ui/kbd-state.h
@@ -0,0 +1,32 @@
+#ifndef QEMU_UI_KBD_STATE_H
+#define QEMU_UI_KBD_STATE_H 1
+
+#include "qapi/qapi-types-ui.h"
+
+typedef enum KbdModifier KbdModifier;
+
+enum KbdModifier {
+    KBD_MOD_NONE = 0,
+
+    KBD_MOD_SHIFT,
+    KBD_MOD_CTRL,
+    KBD_MOD_ALT,
+    KBD_MOD_ALTGR,
+
+    KBD_MOD_NUMLOCK,
+    KBD_MOD_CAPSLOCK,
+
+    KBD_MOD__MAX
+};
+
+typedef struct KbdState KbdState;
+
+bool kbd_state_modifier_get(KbdState *kbd, KbdModifier mod);
+bool kbd_state_key_get(KbdState *kbd, QKeyCode qcode);
+void kbd_state_key_event(KbdState *kbd, QKeyCode qcode, bool down);
+void kbd_state_lift_all_keys(KbdState *kbd);
+void kbd_state_set_delay(KbdState *kbd, int delay_ms);
+void kbd_state_free(KbdState *kbd);
+KbdState *kbd_state_init(QemuConsole *con);
+
+#endif /* QEMU_UI_KBD_STATE_H */
diff --git a/ui/kbd-state.c b/ui/kbd-state.c
new file mode 100644
index 0000000000..00eb1df7fd
--- /dev/null
+++ b/ui/kbd-state.c
@@ -0,0 +1,125 @@
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/queue.h"
+#include "ui/console.h"
+#include "ui/input.h"
+#include "ui/kbd-state.h"
+
+struct KbdState {
+    QemuConsole *con;
+    int key_delay_ms;
+    DECLARE_BITMAP(keys, Q_KEY_CODE__MAX);
+    DECLARE_BITMAP(mods, KBD_MOD__MAX);
+};
+
+static void kbd_state_modifier_update(KbdState *kbd,
+                                      QKeyCode qcode1, QKeyCode qcode2,
+                                      KbdModifier mod)
+{
+    if (test_bit(qcode1, kbd->keys) || test_bit(qcode2, kbd->keys)) {
+        set_bit(mod, kbd->mods);
+    } else {
+        clear_bit(mod, kbd->mods);
+    }
+}
+
+bool kbd_state_modifier_get(KbdState *kbd, KbdModifier mod)
+{
+    return test_bit(mod, kbd->mods);
+}
+
+bool kbd_state_key_get(KbdState *kbd, QKeyCode qcode)
+{
+    return test_bit(qcode, kbd->keys);
+}
+
+void kbd_state_key_event(KbdState *kbd, QKeyCode qcode, bool down)
+{
+    bool state = test_bit(qcode, kbd->keys);
+
+    if (state == down) {
+        /*
+         * Filter out events which don't change the keyboard state.
+         *
+         * Most notably this allows to simply send along all key-up
+         * events, and this function will filter out everything where
+         * the corresponding key-down event wasn't send to the guest,
+         * for example due to being a host hotkey.
+         */
+        return;
+    }
+
+    /* update key and modifier state */
+    change_bit(qcode, kbd->keys);
+    switch (qcode) {
+    case Q_KEY_CODE_SHIFT:
+    case Q_KEY_CODE_SHIFT_R:
+        kbd_state_modifier_update(kbd, Q_KEY_CODE_SHIFT, Q_KEY_CODE_SHIFT_R,
+                                  KBD_MOD_SHIFT);
+        break;
+    case Q_KEY_CODE_CTRL:
+    case Q_KEY_CODE_CTRL_R:
+        kbd_state_modifier_update(kbd, Q_KEY_CODE_CTRL, Q_KEY_CODE_CTRL_R,
+                                  KBD_MOD_CTRL);
+        break;
+    case Q_KEY_CODE_ALT:
+        kbd_state_modifier_update(kbd, Q_KEY_CODE_ALT, Q_KEY_CODE_ALT,
+                                  KBD_MOD_ALT);
+        break;
+    case Q_KEY_CODE_ALT_R:
+        kbd_state_modifier_update(kbd, Q_KEY_CODE_ALT_R, Q_KEY_CODE_ALT_R,
+                                  KBD_MOD_ALTGR);
+        break;
+    case Q_KEY_CODE_CAPS_LOCK:
+        if (down) {
+            change_bit(KBD_MOD_CAPSLOCK, kbd->mods);
+        }
+        break;
+    case Q_KEY_CODE_NUM_LOCK:
+        if (down) {
+            change_bit(KBD_MOD_NUMLOCK, kbd->mods);
+        }
+        break;
+    default:
+        /* keep gcc happy */
+        break;
+    }
+
+    /* send to guest */
+    if (qemu_console_is_graphic(kbd->con)) {
+        qemu_input_event_send_key_qcode(kbd->con, qcode, down);
+        if (kbd->key_delay_ms) {
+            qemu_input_event_send_key_delay(kbd->key_delay_ms);
+        }
+    }
+}
+
+void kbd_state_lift_all_keys(KbdState *kbd)
+{
+    int qcode;
+
+    for (qcode = 0; qcode < Q_KEY_CODE__MAX; qcode++) {
+        if (test_bit(qcode, kbd->keys)) {
+            kbd_state_key_event(kbd, qcode, false);
+        }
+    }
+}
+
+void kbd_state_set_delay(KbdState *kbd, int delay_ms)
+{
+    kbd->key_delay_ms = delay_ms;
+}
+
+void kbd_state_free(KbdState *kbd)
+{
+    g_free(kbd);
+}
+
+KbdState *kbd_state_init(QemuConsole *con)
+{
+    KbdState *kbd = g_new0(KbdState, 1);
+
+    kbd->con = con;
+
+    return kbd;
+}
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 00f6976c30..3c04bdda94 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -8,7 +8,7 @@ vnc-obj-y += vnc-ws.o
 vnc-obj-y += vnc-jobs.o
 
 common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
-common-obj-y += input.o input-keymap.o input-legacy.o
+common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o
 common-obj-$(CONFIG_LINUX) += input-linux.o
 common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
 common-obj-$(CONFIG_COCOA) += cocoa.o
-- 
2.9.3

[Qemu-devel] [RFC PATCH v2 2/7] kbd-state: use state tracker for sdl2

[Gerd Hoffmann]

Use the new keyboard state tracked for sdl2.  We can drop the modifier
state tracking from sdl2.  Also keyup code is simpler, the state tracker
will take care to not send suspious keyup events to the guest.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/sdl2.h |  2 ++
 ui/sdl2-input.c   | 40 +++-------------------------------------
 ui/sdl2.c         | 12 +++---------
 3 files changed, 8 insertions(+), 46 deletions(-)

diff --git a/include/ui/sdl2.h b/include/ui/sdl2.h
index f43eecdbd6..393f4b0fd5 100644
--- a/include/ui/sdl2.h
+++ b/include/ui/sdl2.h
@@ -7,6 +7,7 @@
 #include <SDL.h>
 #include <SDL_syswm.h>
 
+#include "ui/kbd-state.h"
 #ifdef CONFIG_OPENGL
 # include "ui/egl-helpers.h"
 #endif
@@ -27,6 +28,7 @@ struct sdl2_console {
     int idle_counter;
     int ignore_hotkeys;
     SDL_GLContext winctx;
+    KbdState *kbd;
 #ifdef CONFIG_OPENGL
     QemuGLShader *gls;
     egl_fb guest_fb;
diff --git a/ui/sdl2-input.c b/ui/sdl2-input.c
index 1378b63dd9..ae52d548f6 100644
--- a/ui/sdl2-input.c
+++ b/ui/sdl2-input.c
@@ -30,22 +30,9 @@
 #include "ui/sdl2.h"
 #include "sysemu/sysemu.h"
 
-static uint8_t modifiers_state[SDL_NUM_SCANCODES];
-
 void sdl2_reset_keys(struct sdl2_console *scon)
 {
-    QemuConsole *con = scon ? scon->dcl.con : NULL;
-    int i;
-
-    for (i = 0 ;
-         i < SDL_NUM_SCANCODES && i < qemu_input_map_usb_to_qcode_len ;
-         i++) {
-        if (modifiers_state[i]) {
-            int qcode = qemu_input_map_usb_to_qcode[i];
-            qemu_input_event_send_key_qcode(con, qcode, false);
-            modifiers_state[i] = 0;
-        }
-    }
+    kbd_state_lift_all_keys(scon->kbd);
 }
 
 void sdl2_process_key(struct sdl2_console *scon,
@@ -59,31 +46,10 @@ void sdl2_process_key(struct sdl2_console *scon,
     }
 
     qcode = qemu_input_map_usb_to_qcode[ev->keysym.scancode];
-
-    /* modifier state tracking */
-    switch (ev->keysym.scancode) {
-    case SDL_SCANCODE_LCTRL:
-    case SDL_SCANCODE_LSHIFT:
-    case SDL_SCANCODE_LALT:
-    case SDL_SCANCODE_LGUI:
-    case SDL_SCANCODE_RCTRL:
-    case SDL_SCANCODE_RSHIFT:
-    case SDL_SCANCODE_RALT:
-    case SDL_SCANCODE_RGUI:
-        if (ev->type == SDL_KEYUP) {
-            modifiers_state[ev->keysym.scancode] = 0;
-        } else {
-            modifiers_state[ev->keysym.scancode] = 1;
-        }
-        break;
-    default:
-        /* nothing */
-        break;
-    }
+    kbd_state_key_event(scon->kbd, qcode, ev->type == SDL_KEYDOWN);
 
     if (!qemu_console_is_graphic(con)) {
-        bool ctrl = (modifiers_state[SDL_SCANCODE_LCTRL] ||
-                     modifiers_state[SDL_SCANCODE_RCTRL]);
+        bool ctrl = kbd_state_modifier_get(scon->kbd, KBD_MOD_CTRL);
         if (ev->type == SDL_KEYDOWN) {
             switch (ev->keysym.scancode) {
             case SDL_SCANCODE_RETURN:
diff --git a/ui/sdl2.c b/ui/sdl2.c
index a10b6e3a08..ee1a2445d6 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -38,7 +38,6 @@ static int gui_grab; /* if true, all keyboard/mouse events are grabbed */
 
 static int gui_saved_grab;
 static int gui_fullscreen;
-static int gui_keysym;
 static int gui_grab_code = KMOD_LALT | KMOD_LCTRL;
 static SDL_Cursor *sdl_cursor_normal;
 static SDL_Cursor *sdl_cursor_hidden;
@@ -330,6 +329,7 @@ static void handle_keydown(SDL_Event *ev)
     int win;
     struct sdl2_console *scon = get_scon_from_window(ev->key.windowID);
     int gui_key_modifier_pressed = get_mod_state();
+    int gui_keysym = 0;
 
     if (!scon->ignore_hotkeys && gui_key_modifier_pressed && !ev->key.repeat) {
         switch (ev->key.keysym.scancode) {
@@ -410,16 +410,9 @@ static void handle_keydown(SDL_Event *ev)
 static void handle_keyup(SDL_Event *ev)
 {
     struct sdl2_console *scon = get_scon_from_window(ev->key.windowID);
-    int gui_key_modifier_pressed = get_mod_state();
 
     scon->ignore_hotkeys = false;
-
-    if (!gui_key_modifier_pressed) {
-        gui_keysym = 0;
-    }
-    if (!gui_keysym) {
-        sdl2_process_key(scon, &ev->key);
-    }
+    sdl2_process_key(scon, &ev->key);
 }
 
 static void handle_textinput(SDL_Event *ev)
@@ -823,6 +816,7 @@ static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
         sdl2_console[i].dcl.ops = &dcl_2d_ops;
 #endif
         sdl2_console[i].dcl.con = con;
+        sdl2_console[i].kbd = kbd_state_init(con);
         register_displaychangelistener(&sdl2_console[i].dcl);
 
 #if defined(SDL_VIDEO_DRIVER_WINDOWS) || defined(SDL_VIDEO_DRIVER_X11)
-- 
2.9.3

[Qemu-devel] [RFC PATCH v2 3/7] sdl2: use only QKeyCode in sdl2_process_key()

[Gerd Hoffmann]

Small cleanup.  Also drop the special backspace handling,
kbd_put_qcode_console() learned to handle that meanwhile.
And sdl2_process_key is never called with scon == NULL.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/sdl2-input.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ui/sdl2-input.c b/ui/sdl2-input.c
index ae52d548f6..78241c2a98 100644
--- a/ui/sdl2-input.c
+++ b/ui/sdl2-input.c
@@ -39,20 +39,19 @@ void sdl2_process_key(struct sdl2_console *scon,
                       SDL_KeyboardEvent *ev)
 {
     int qcode;
-    QemuConsole *con = scon ? scon->dcl.con : NULL;
+    QemuConsole *con = scon->dcl.con;
 
     if (ev->keysym.scancode >= qemu_input_map_usb_to_qcode_len) {
         return;
     }
-
     qcode = qemu_input_map_usb_to_qcode[ev->keysym.scancode];
     kbd_state_key_event(scon->kbd, qcode, ev->type == SDL_KEYDOWN);
 
     if (!qemu_console_is_graphic(con)) {
         bool ctrl = kbd_state_modifier_get(scon->kbd, KBD_MOD_CTRL);
         if (ev->type == SDL_KEYDOWN) {
-            switch (ev->keysym.scancode) {
-            case SDL_SCANCODE_RETURN:
+            switch (qcode) {
+            case Q_KEY_CODE_RET:
                 kbd_put_keysym_console(con, '\n');
                 break;
             default:
-- 
2.9.3

[Qemu-devel] [RFC PATCH v2 4/7] kbd-state: use state tracker for gtk

[Gerd Hoffmann]

Use the new keyboard state tracked for gtk.  Allows to drop the
gtk-specific modifier state tracking code.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/gtk.h |  2 ++
 ui/gtk.c         | 38 ++++++--------------------------------
 2 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index 99edd3c085..9cf0c9e55c 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -22,6 +22,7 @@
 #include <gdk/gdkwayland.h>
 #endif
 
+#include "ui/kbd-state.h"
 #if defined(CONFIG_OPENGL)
 #include "ui/egl-helpers.h"
 #include "ui/egl-context.h"
@@ -32,6 +33,7 @@ typedef struct GtkDisplayState GtkDisplayState;
 typedef struct VirtualGfxConsole {
     GtkWidget *drawing_area;
     DisplayChangeListener dcl;
+    KbdState *kbd;
     DisplaySurface *ds;
     pixman_image_t *convert;
     cairo_surface_t *surface;
diff --git a/ui/gtk.c b/ui/gtk.c
index 579990b865..85d095e624 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -122,17 +122,6 @@
 
 #define HOTKEY_MODIFIERS        (GDK_CONTROL_MASK | GDK_MOD1_MASK)
 
-static const int modifier_keycode[] = {
-    Q_KEY_CODE_SHIFT,
-    Q_KEY_CODE_SHIFT_R,
-    Q_KEY_CODE_CTRL,
-    Q_KEY_CODE_CTRL_R,
-    Q_KEY_CODE_ALT,
-    Q_KEY_CODE_ALT_R,
-    Q_KEY_CODE_META_L,
-    Q_KEY_CODE_META_R,
-};
-
 static const guint16 *keycode_map;
 static size_t keycode_maplen;
 
@@ -187,7 +176,6 @@ struct GtkDisplayState {
 
     bool external_pause_update;
 
-    bool modifier_pressed[ARRAY_SIZE(modifier_keycode)];
     bool ignore_keys;
 
     DisplayOptions *opts;
@@ -426,20 +414,12 @@ static void gd_update_full_redraw(VirtualConsole *vc)
 static void gtk_release_modifiers(GtkDisplayState *s)
 {
     VirtualConsole *vc = gd_vc_find_current(s);
-    int i, qcode;
 
     if (vc->type != GD_VC_GFX ||
         !qemu_console_is_graphic(vc->gfx.dcl.con)) {
         return;
     }
-    for (i = 0; i < ARRAY_SIZE(modifier_keycode); i++) {
-        qcode = modifier_keycode[i];
-        if (!s->modifier_pressed[i]) {
-            continue;
-        }
-        qemu_input_event_send_key_qcode(vc->gfx.dcl.con, qcode, false);
-        s->modifier_pressed[i] = false;
-    }
+    kbd_state_lift_all_keys(vc->gfx.kbd);
 }
 
 static void gd_widget_reparent(GtkWidget *from, GtkWidget *to,
@@ -1113,7 +1093,6 @@ static gboolean gd_key_event(GtkWidget *widget, GdkEventKey *key, void *opaque)
     VirtualConsole *vc = opaque;
     GtkDisplayState *s = vc->s;
     int qcode;
-    int i;
 
     if (s->ignore_keys) {
         s->ignore_keys = (key->type == GDK_KEY_PRESS);
@@ -1134,8 +1113,8 @@ static gboolean gd_key_event(GtkWidget *widget, GdkEventKey *key, void *opaque)
         || key->hardware_keycode == VK_PAUSE
 #endif
         ) {
-        qemu_input_event_send_key_qcode(vc->gfx.dcl.con, Q_KEY_CODE_PAUSE,
-                                        key->type == GDK_KEY_PRESS);
+        kbd_state_key_event(vc->gfx.kbd, Q_KEY_CODE_PAUSE,
+                            key->type == GDK_KEY_PRESS);
         return TRUE;
     }
 
@@ -1144,14 +1123,8 @@ static gboolean gd_key_event(GtkWidget *widget, GdkEventKey *key, void *opaque)
     trace_gd_key_event(vc->label, key->hardware_keycode, qcode,
                        (key->type == GDK_KEY_PRESS) ? "down" : "up");
 
-    for (i = 0; i < ARRAY_SIZE(modifier_keycode); i++) {
-        if (qcode == modifier_keycode[i]) {
-            s->modifier_pressed[i] = (key->type == GDK_KEY_PRESS);
-        }
-    }
-
-    qemu_input_event_send_key_qcode(vc->gfx.dcl.con, qcode,
-                                    key->type == GDK_KEY_PRESS);
+    kbd_state_key_event(vc->gfx.kbd, qcode,
+                        key->type == GDK_KEY_PRESS);
 
     return TRUE;
 }
@@ -2052,6 +2025,7 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s, VirtualConsole *vc,
     gtk_notebook_append_page(GTK_NOTEBOOK(s->notebook),
                              vc->tab_item, gtk_label_new(vc->label));
 
+    vc->gfx.kbd = kbd_state_init(con);
     vc->gfx.dcl.con = con;
     register_displaychangelistener(&vc->gfx.dcl);
 
-- 
2.9.3

[Qemu-devel] [RFC PATCH v2 5/7] kbd-state: use state tracker for vnc

[Gerd Hoffmann]

Use the new keyboard state tracked for vnc.  Allows to drop the
vnc-specific modifier state tracking code.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.h |   5 ++-
 ui/vnc.c | 120 ++++++++++++++++++---------------------------------------------
 2 files changed, 35 insertions(+), 90 deletions(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index a86e0610e8..56111908ce 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -44,6 +44,7 @@
 #include "keymaps.h"
 #include "vnc-palette.h"
 #include "vnc-enc-zrle.h"
+#include "ui/kbd-state.h"
 
 // #define _VNC_DEBUG 1
 
@@ -155,7 +156,7 @@ struct VncDisplay
     int lock_key_sync;
     QEMUPutLEDEntry *led;
     int ledstate;
-    int key_delay_ms;
+    KbdState *kbd;
     QemuMutex mutex;
 
     QEMUCursor *cursor;
@@ -326,8 +327,6 @@ struct VncState
 
     VncReadEvent *read_handler;
     size_t read_handler_expect;
-    /* input */
-    uint8_t modifiers_state[256];
 
     bool abort;
     QemuMutex output_mutex;
diff --git a/ui/vnc.c b/ui/vnc.c
index 0c1b477425..b56431ce3b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -59,7 +59,6 @@ static QTAILQ_HEAD(, VncDisplay) vnc_displays =
     QTAILQ_HEAD_INITIALIZER(vnc_displays);
 
 static int vnc_cursor_define(VncState *vs);
-static void vnc_release_modifiers(VncState *vs);
 static void vnc_update_throttle_offset(VncState *vs);
 
 static void vnc_set_share_mode(VncState *vs, VncShareMode mode)
@@ -1248,7 +1247,7 @@ void vnc_disconnect_finish(VncState *vs)
     vnc_sasl_client_cleanup(vs);
 #endif /* CONFIG_VNC_SASL */
     audio_del(vs);
-    vnc_release_modifiers(vs);
+    kbd_state_lift_all_keys(vs->vd->kbd);
 
     if (vs->mouse_mode_notifier.notify != NULL) {
         qemu_remove_mouse_mode_change_notifier(&vs->mouse_mode_notifier);
@@ -1737,26 +1736,10 @@ static void pointer_event(VncState *vs, int button_mask, int x, int y)
     qemu_input_event_sync();
 }
 
-static void reset_keys(VncState *vs)
+static void press_key(VncState *vs, QKeyCode qcode)
 {
-    int i;
-    for(i = 0; i < 256; i++) {
-        if (vs->modifiers_state[i]) {
-            qemu_input_event_send_key_number(vs->vd->dcl.con, i, false);
-            qemu_input_event_send_key_delay(vs->vd->key_delay_ms);
-            vs->modifiers_state[i] = 0;
-        }
-    }
-}
-
-static void press_key(VncState *vs, int keysym)
-{
-    int keycode = keysym2scancode(vs->vd->kbd_layout, keysym,
-                                  false, false, false) & SCANCODE_KEYMASK;
-    qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, true);
-    qemu_input_event_send_key_delay(vs->vd->key_delay_ms);
-    qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, false);
-    qemu_input_event_send_key_delay(vs->vd->key_delay_ms);
+    kbd_state_key_event(vs->vd->kbd, qcode, true);
+    kbd_state_key_event(vs->vd->kbd, qcode, false);
 }
 
 static void vnc_led_state_change(VncState *vs)
@@ -1797,32 +1780,20 @@ static void kbd_leds(void *opaque, int ledstate)
 
 static void do_key_event(VncState *vs, int down, int keycode, int sym)
 {
+    QKeyCode qcode = qemu_input_key_number_to_qcode(keycode);
+
     /* QEMU console switch */
-    switch(keycode) {
-    case 0x2a:                          /* Left Shift */
-    case 0x36:                          /* Right Shift */
-    case 0x1d:                          /* Left CTRL */
-    case 0x9d:                          /* Right CTRL */
-    case 0x38:                          /* Left ALT */
-    case 0xb8:                          /* Right ALT */
-        if (down)
-            vs->modifiers_state[keycode] = 1;
-        else
-            vs->modifiers_state[keycode] = 0;
-        break;
-    case 0x02 ... 0x0a: /* '1' to '9' keys */
-        if (vs->vd->dcl.con == NULL &&
-            down && vs->modifiers_state[0x1d] && vs->modifiers_state[0x38]) {
+    switch (qcode) {
+    case Q_KEY_CODE_1 ... Q_KEY_CODE_9: /* '1' to '9' keys */
+        if (vs->vd->dcl.con == NULL && down &&
+            kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CTRL) &&
+            kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_ALT)) {
             /* Reset the modifiers sent to the current console */
-            reset_keys(vs);
-            console_select(keycode - 0x02);
+            kbd_state_lift_all_keys(vs->vd->kbd);
+            console_select(qcode - Q_KEY_CODE_1);
             return;
         }
-        break;
-    case 0x3a:                        /* CapsLock */
-    case 0x45:                        /* NumLock */
-        if (down)
-            vs->modifiers_state[keycode] ^= 1;
+    default:
         break;
     }
 
@@ -1837,16 +1808,14 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
            toggles numlock away from the VNC window.
         */
         if (keysym_is_numlock(vs->vd->kbd_layout, sym & 0xFFFF)) {
-            if (!vs->modifiers_state[0x45]) {
+            if (!kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_NUMLOCK)) {
                 trace_vnc_key_sync_numlock(true);
-                vs->modifiers_state[0x45] = 1;
-                press_key(vs, 0xff7f);
+                press_key(vs, Q_KEY_CODE_NUM_LOCK);
             }
         } else {
-            if (vs->modifiers_state[0x45]) {
+            if (kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_NUMLOCK)) {
                 trace_vnc_key_sync_numlock(false);
-                vs->modifiers_state[0x45] = 0;
-                press_key(vs, 0xff7f);
+                press_key(vs, Q_KEY_CODE_NUM_LOCK);
             }
         }
     }
@@ -1859,30 +1828,25 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
            toggles capslock away from the VNC window.
         */
         int uppercase = !!(sym >= 'A' && sym <= 'Z');
-        int shift = !!(vs->modifiers_state[0x2a] | vs->modifiers_state[0x36]);
-        int capslock = !!(vs->modifiers_state[0x3a]);
+        bool shift = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_SHIFT);
+        bool capslock = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CAPSLOCK);
         if (capslock) {
             if (uppercase == shift) {
                 trace_vnc_key_sync_capslock(false);
-                vs->modifiers_state[0x3a] = 0;
-                press_key(vs, 0xffe5);
+                press_key(vs, Q_KEY_CODE_CAPS_LOCK);
             }
         } else {
             if (uppercase != shift) {
                 trace_vnc_key_sync_capslock(true);
-                vs->modifiers_state[0x3a] = 1;
-                press_key(vs, 0xffe5);
+                press_key(vs, Q_KEY_CODE_CAPS_LOCK);
             }
         }
     }
 
-    if (qemu_console_is_graphic(NULL)) {
-        qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, down);
-        qemu_input_event_send_key_delay(vs->vd->key_delay_ms);
-    } else {
-        bool numlock = vs->modifiers_state[0x45];
-        bool control = (vs->modifiers_state[0x1d] ||
-                        vs->modifiers_state[0x9d]);
+    kbd_state_key_event(vs->vd->kbd, qcode, down);
+    if (!qemu_console_is_graphic(NULL)) {
+        bool numlock = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_NUMLOCK);
+        bool control = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CTRL);
         /* QEMU console emulation */
         if (down) {
             switch (keycode) {
@@ -1983,27 +1947,6 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
     }
 }
 
-static void vnc_release_modifiers(VncState *vs)
-{
-    static const int keycodes[] = {
-        /* shift, control, alt keys, both left & right */
-        0x2a, 0x36, 0x1d, 0x9d, 0x38, 0xb8,
-    };
-    int i, keycode;
-
-    if (!qemu_console_is_graphic(NULL)) {
-        return;
-    }
-    for (i = 0; i < ARRAY_SIZE(keycodes); i++) {
-        keycode = keycodes[i];
-        if (!vs->modifiers_state[keycode]) {
-            continue;
-        }
-        qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, false);
-        qemu_input_event_send_key_delay(vs->vd->key_delay_ms);
-    }
-}
-
 static const char *code2name(int keycode)
 {
     return QKeyCode_str(qemu_input_key_number_to_qcode(keycode));
@@ -2011,9 +1954,9 @@ static const char *code2name(int keycode)
 
 static void key_event(VncState *vs, int down, uint32_t sym)
 {
-    bool shift = vs->modifiers_state[0x2a] || vs->modifiers_state[0x36];
-    bool altgr = vs->modifiers_state[0xb8];
-    bool ctrl  = vs->modifiers_state[0x1d] || vs->modifiers_state[0x9d];
+    bool shift = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_SHIFT);
+    bool altgr = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_ALTGR);
+    bool ctrl  = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CTRL);
     int keycode;
     int lsym = sym;
 
@@ -3240,6 +3183,7 @@ void vnc_display_init(const char *id, Error **errp)
 
     vd->dcl.ops = &dcl_ops;
     register_displaychangelistener(&vd->dcl);
+    vd->kbd = kbd_state_init(vd->dcl.con);
 }
 
 
@@ -3976,7 +3920,6 @@ void vnc_display_open(const char *id, Error **errp)
         vd->led = qemu_add_led_event_handler(kbd_leds, vd);
     }
     vd->ledstate = 0;
-    vd->key_delay_ms = key_delay_ms;
 
     device_id = qemu_opt_get(opts, "display");
     if (device_id) {
@@ -3993,10 +3936,13 @@ void vnc_display_open(const char *id, Error **errp)
     }
 
     if (con != vd->dcl.con) {
+        kbd_state_free(vd->kbd);
         unregister_displaychangelistener(&vd->dcl);
         vd->dcl.con = con;
         register_displaychangelistener(&vd->dcl);
+        vd->kbd = kbd_state_init(vd->dcl.con);
     }
+    kbd_state_set_delay(vd->kbd, key_delay_ms);
 
     if (saddr == NULL) {
         goto cleanup;
-- 
2.9.3

[Qemu-devel] [RFC PATCH v2 6/7] keymap: pass full keyboard state to keysym2scancode

[Gerd Hoffmann]

Pass the keyboard state tracker handle down to keysym2scancode(),
so the code can fully inspect the keyboard state as needed.  No
functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/keymaps.h | 3 ++-
 ui/curses.c  | 2 +-
 ui/keymaps.c | 8 ++++----
 ui/vnc.c     | 5 +----
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/ui/keymaps.h b/ui/keymaps.h
index 98213a4191..6a2365196a 100644
--- a/ui/keymaps.h
+++ b/ui/keymaps.h
@@ -26,6 +26,7 @@
 #define QEMU_KEYMAPS_H
 
 #include "qemu-common.h"
+#include "ui/kbd-state.h"
 
 typedef struct {
 	const char* name;
@@ -55,7 +56,7 @@ typedef struct kbd_layout_t kbd_layout_t;
 kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
                                    const char *language, Error **errp);
 int keysym2scancode(kbd_layout_t *k, int keysym,
-                    bool shift, bool altgr, bool ctrl);
+                    KbdState *kbd);
 int keycode_is_keypad(kbd_layout_t *k, int keycode);
 int keysym_is_numlock(kbd_layout_t *k, int keysym);
 
diff --git a/ui/curses.c b/ui/curses.c
index f4e7a12f74..54687589d4 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -273,7 +273,7 @@ static void curses_refresh(DisplayChangeListener *dcl)
             }
 
             keycode = keysym2scancode(kbd_layout, keysym & KEYSYM_MASK,
-                                      false, false, false);
+                                      NULL);
             if (keycode == 0)
                 continue;
 
diff --git a/ui/keymaps.c b/ui/keymaps.c
index 085889b555..6be87cc201 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -189,7 +189,7 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
 
 
 int keysym2scancode(kbd_layout_t *k, int keysym,
-                    bool shift, bool altgr, bool ctrl)
+                    KbdState *kbd)
 {
     static const uint32_t mask =
         SCANCODE_SHIFT | SCANCODE_ALTGR | SCANCODE_CTRL;
@@ -221,13 +221,13 @@ int keysym2scancode(kbd_layout_t *k, int keysym,
      * If so, prefer that one.
      */
     mods = 0;
-    if (shift) {
+    if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_SHIFT)) {
         mods |= SCANCODE_SHIFT;
     }
-    if (altgr) {
+    if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_ALTGR)) {
         mods |= SCANCODE_ALTGR;
     }
-    if (ctrl) {
+    if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_CTRL)) {
         mods |= SCANCODE_CTRL;
     }
 
diff --git a/ui/vnc.c b/ui/vnc.c
index b56431ce3b..704a138667 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1954,9 +1954,6 @@ static const char *code2name(int keycode)
 
 static void key_event(VncState *vs, int down, uint32_t sym)
 {
-    bool shift = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_SHIFT);
-    bool altgr = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_ALTGR);
-    bool ctrl  = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CTRL);
     int keycode;
     int lsym = sym;
 
@@ -1965,7 +1962,7 @@ static void key_event(VncState *vs, int down, uint32_t sym)
     }
 
     keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF,
-                              shift, altgr, ctrl) & SCANCODE_KEYMASK;
+                              vs->vd->kbd) & SCANCODE_KEYMASK;
     trace_vnc_key_event_map(down, sym, keycode, code2name(keycode));
     do_key_event(vs, down, keycode, sym);
 }
-- 
2.9.3

[Qemu-devel] [RFC PATCH v2 7/7] keymap: fix keyup mappings

[Gerd Hoffmann]

It is possible that the modifier state on keyup is different from the
modifier state on keydown.  In that case the keycode lookup can end up
with different keys in case multiple keysym -> keycode mappings exist,
because it picks the mapping depending on modifier state.

To fix that change the lookup logic for keyup events.  Instead of
looking at the modifier state check the key state and prefer a keycodes
where the key is in "down" state right now.

Fixes: abb4f2c965 keymap: consider modifier state when picking a mapping
Buglink: https://bugs.launchpad.net/bugs/1738283
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1658676
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/keymaps.h |  2 +-
 ui/curses.c  |  2 +-
 ui/keymaps.c | 55 ++++++++++++++++++++++++++++++++++---------------------
 ui/vnc.c     |  2 +-
 4 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/ui/keymaps.h b/ui/keymaps.h
index 6a2365196a..97deec3437 100644
--- a/ui/keymaps.h
+++ b/ui/keymaps.h
@@ -56,7 +56,7 @@ typedef struct kbd_layout_t kbd_layout_t;
 kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
                                    const char *language, Error **errp);
 int keysym2scancode(kbd_layout_t *k, int keysym,
-                    KbdState *kbd);
+                    KbdState *kbd, bool down);
 int keycode_is_keypad(kbd_layout_t *k, int keycode);
 int keysym_is_numlock(kbd_layout_t *k, int keysym);
 
diff --git a/ui/curses.c b/ui/curses.c
index 54687589d4..6e0091c3b2 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -273,7 +273,7 @@ static void curses_refresh(DisplayChangeListener *dcl)
             }
 
             keycode = keysym2scancode(kbd_layout, keysym & KEYSYM_MASK,
-                                      NULL);
+                                      NULL, false);
             if (keycode == 0)
                 continue;
 
diff --git a/ui/keymaps.c b/ui/keymaps.c
index 6be87cc201..9ec0bc5333 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -28,6 +28,7 @@
 #include "trace.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "ui/input.h"
 
 struct keysym2code {
     uint32_t count;
@@ -189,7 +190,7 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
 
 
 int keysym2scancode(kbd_layout_t *k, int keysym,
-                    KbdState *kbd)
+                    KbdState *kbd, bool down)
 {
     static const uint32_t mask =
         SCANCODE_SHIFT | SCANCODE_ALTGR | SCANCODE_CTRL;
@@ -213,27 +214,39 @@ int keysym2scancode(kbd_layout_t *k, int keysym,
         return keysym2code->keycodes[0];
     }
 
-    /*
-     * We have multiple keysym -> keycode mappings.
-     *
-     * Check whenever we find one mapping where the modifier state of
-     * the mapping matches the current user interface modifier state.
-     * If so, prefer that one.
-     */
-    mods = 0;
-    if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_SHIFT)) {
-        mods |= SCANCODE_SHIFT;
-    }
-    if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_ALTGR)) {
-        mods |= SCANCODE_ALTGR;
-    }
-    if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_CTRL)) {
-        mods |= SCANCODE_CTRL;
-    }
+    /* We have multiple keysym -> keycode mappings. */
+    if (down) {
+        /*
+         * On keydown: Check whenever we find one mapping where the
+         * modifier state of the mapping matches the current user
+         * interface modifier state.  If so, prefer that one.
+         */
+        mods = 0;
+        if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_SHIFT)) {
+            mods |= SCANCODE_SHIFT;
+        }
+        if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_ALTGR)) {
+            mods |= SCANCODE_ALTGR;
+        }
+        if (kbd && kbd_state_modifier_get(kbd, KBD_MOD_CTRL)) {
+            mods |= SCANCODE_CTRL;
+        }
 
-    for (i = 0; i < keysym2code->count; i++) {
-        if ((keysym2code->keycodes[i] & mask) == mods) {
-            return keysym2code->keycodes[i];
+        for (i = 0; i < keysym2code->count; i++) {
+            if ((keysym2code->keycodes[i] & mask) == mods) {
+                return keysym2code->keycodes[i];
+            }
+        }
+    } else {
+        /*
+         * On keyup: Try find a key which is actually down.
+         */
+        for (i = 0; i < keysym2code->count; i++) {
+            QKeyCode qcode = qemu_input_key_number_to_qcode
+                (keysym2code->keycodes[i]);
+            if (kbd && kbd_state_key_get(kbd, qcode)) {
+                return keysym2code->keycodes[i];
+            }
         }
     }
     return keysym2code->keycodes[0];
diff --git a/ui/vnc.c b/ui/vnc.c
index 704a138667..08eba8b6ed 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1962,7 +1962,7 @@ static void key_event(VncState *vs, int down, uint32_t sym)
     }
 
     keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF,
-                              vs->vd->kbd) & SCANCODE_KEYMASK;
+                              vs->vd->kbd, down) & SCANCODE_KEYMASK;
     trace_vnc_key_event_map(down, sym, keycode, code2name(keycode));
     do_key_event(vs, down, keycode, sym);
 }
-- 
2.9.3

Re: [Qemu-devel] [RFC PATCH v2 1/7] kbd-state: add keyboard state tracker

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 01:08:58PM +0100, Gerd Hoffmann wrote:
> Now that most user interfaces are using QKeyCodes it is easier to have
> common keyboard code useable by all user interfaces.
> 
> This patch adds helper code to track the state of all keyboard keys,
> using a bitmap indexed by QKeyCode.  Modifier state is tracked too,
> as separate bitmap.  That makes checking modifier state easier.
> Likewise we can easily apply special handling for capslock & numlock
> (toggles on keypress) and ctrl + shift (we have two keys for that).
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/ui/kbd-state.h |  32 +++++++++++++
>  ui/kbd-state.c         | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
>  ui/Makefile.objs       |   2 +-
>  3 files changed, 158 insertions(+), 1 deletion(-)
>  create mode 100644 include/ui/kbd-state.h
>  create mode 100644 ui/kbd-state.c
> 
> diff --git a/include/ui/kbd-state.h b/include/ui/kbd-state.h
> new file mode 100644
> index 0000000000..0bef75a5d5
> --- /dev/null
> +++ b/include/ui/kbd-state.h
> @@ -0,0 +1,32 @@
> +#ifndef QEMU_UI_KBD_STATE_H
> +#define QEMU_UI_KBD_STATE_H 1
> +
> +#include "qapi/qapi-types-ui.h"
> +
> +typedef enum KbdModifier KbdModifier;
> +
> +enum KbdModifier {
> +    KBD_MOD_NONE = 0,
> +
> +    KBD_MOD_SHIFT,
> +    KBD_MOD_CTRL,
> +    KBD_MOD_ALT,
> +    KBD_MOD_ALTGR,
> +
> +    KBD_MOD_NUMLOCK,
> +    KBD_MOD_CAPSLOCK,
> +
> +    KBD_MOD__MAX
> +};
> +
> +typedef struct KbdState KbdState;
> +
> +bool kbd_state_modifier_get(KbdState *kbd, KbdModifier mod);
> +bool kbd_state_key_get(KbdState *kbd, QKeyCode qcode);
> +void kbd_state_key_event(KbdState *kbd, QKeyCode qcode, bool down);
> +void kbd_state_lift_all_keys(KbdState *kbd);
> +void kbd_state_set_delay(KbdState *kbd, int delay_ms);
> +void kbd_state_free(KbdState *kbd);
> +KbdState *kbd_state_init(QemuConsole *con);

I would suggest using a slightly more QEMU specific namespace "qkbd_"
methods, m"QKBD_" constants, & "QKbd" struct, as 'kbd' is very generic,
but upto you if you want to, or ignore it.

I would, however, like to see API docs written in the header file for
each of these methods.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH v2 2/7] kbd-state: use state tracker for sdl2

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 01:08:59PM +0100, Gerd Hoffmann wrote:
> Use the new keyboard state tracked for sdl2.  We can drop the modifier
> state tracking from sdl2.  Also keyup code is simpler, the state tracker
> will take care to not send suspious keyup events to the guest.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/ui/sdl2.h |  2 ++
>  ui/sdl2-input.c   | 40 +++-------------------------------------
>  ui/sdl2.c         | 12 +++---------
>  3 files changed, 8 insertions(+), 46 deletions(-)
> 
> diff --git a/include/ui/sdl2.h b/include/ui/sdl2.h
> index f43eecdbd6..393f4b0fd5 100644
> --- a/include/ui/sdl2.h
> +++ b/include/ui/sdl2.h
> @@ -7,6 +7,7 @@

> diff --git a/ui/sdl2-input.c b/ui/sdl2-input.c
> index 1378b63dd9..ae52d548f6 100644
> --- a/ui/sdl2-input.c
> +++ b/ui/sdl2-input.c
> @@ -30,22 +30,9 @@
>  #include "ui/sdl2.h"
>  #include "sysemu/sysemu.h"
>  
> -static uint8_t modifiers_state[SDL_NUM_SCANCODES];
> -
>  void sdl2_reset_keys(struct sdl2_console *scon)
>  {
> -    QemuConsole *con = scon ? scon->dcl.con : NULL;
> -    int i;
> -
> -    for (i = 0 ;
> -         i < SDL_NUM_SCANCODES && i < qemu_input_map_usb_to_qcode_len ;
> -         i++) {
> -        if (modifiers_state[i]) {
> -            int qcode = qemu_input_map_usb_to_qcode[i];
> -            qemu_input_event_send_key_qcode(con, qcode, false);
> -            modifiers_state[i] = 0;
> -        }
> -    }
> +    kbd_state_lift_all_keys(scon->kbd);
>  }

I was wondering if we could just make the callers of sdl2_reset_keys
call  kbd_state_lift_all_keys directly. I can't find any callers of
sdl2_reset_keys at all though. It appears you removed all callers
in

  commit f8d2c9369b8302f65f4f43f14ed3987c2268a02a
  Author: Gerd Hoffmann <kraxel@redhat.com>
  Date:   Mon Jan 15 16:48:54 2018 +0100

    sdl: use ctrl-alt-g as grab hotkey

So I guess we could have a prior patch that just deletes this method
entirely.


For the rest of the patch

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH v2 3/7] sdl2: use only QKeyCode in sdl2_process_key()

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 01:09:00PM +0100, Gerd Hoffmann wrote:
> Small cleanup.  Also drop the special backspace handling,
> kbd_put_qcode_console() learned to handle that meanwhile.

I'm not seeing what backspace handlking is changed in this
patch - i guess this comment is left over from a previous
version of the patch.

> And sdl2_process_key is never called with scon == NULL.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/sdl2-input.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/ui/sdl2-input.c b/ui/sdl2-input.c
> index ae52d548f6..78241c2a98 100644
> --- a/ui/sdl2-input.c
> +++ b/ui/sdl2-input.c
> @@ -39,20 +39,19 @@ void sdl2_process_key(struct sdl2_console *scon,
>                        SDL_KeyboardEvent *ev)
>  {
>      int qcode;
> -    QemuConsole *con = scon ? scon->dcl.con : NULL;
> +    QemuConsole *con = scon->dcl.con;
>  
>      if (ev->keysym.scancode >= qemu_input_map_usb_to_qcode_len) {
>          return;
>      }
> -
>      qcode = qemu_input_map_usb_to_qcode[ev->keysym.scancode];
>      kbd_state_key_event(scon->kbd, qcode, ev->type == SDL_KEYDOWN);
>  
>      if (!qemu_console_is_graphic(con)) {
>          bool ctrl = kbd_state_modifier_get(scon->kbd, KBD_MOD_CTRL);
>          if (ev->type == SDL_KEYDOWN) {
> -            switch (ev->keysym.scancode) {
> -            case SDL_SCANCODE_RETURN:
> +            switch (qcode) {
> +            case Q_KEY_CODE_RET:
>                  kbd_put_keysym_console(con, '\n');
>                  break;
>              default:

With fixed commit message

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH v2 4/7] kbd-state: use state tracker for gtk

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 01:09:01PM +0100, Gerd Hoffmann wrote:
> Use the new keyboard state tracked for gtk.  Allows to drop the
> gtk-specific modifier state tracking code.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/ui/gtk.h |  2 ++
>  ui/gtk.c         | 38 ++++++--------------------------------
>  2 files changed, 8 insertions(+), 32 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH v2 5/7] kbd-state: use state tracker for vnc

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 01:09:02PM +0100, Gerd Hoffmann wrote:
> Use the new keyboard state tracked for vnc.  Allows to drop the
> vnc-specific modifier state tracking code.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/vnc.h |   5 ++-
>  ui/vnc.c | 120 ++++++++++++++++++---------------------------------------------
>  2 files changed, 35 insertions(+), 90 deletions(-)
> 
> diff --git a/ui/vnc.h b/ui/vnc.h
> index a86e0610e8..56111908ce 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -44,6 +44,7 @@
>  #include "keymaps.h"
>  #include "vnc-palette.h"
>  #include "vnc-enc-zrle.h"
> +#include "ui/kbd-state.h"
>  
>  // #define _VNC_DEBUG 1
>  
> @@ -155,7 +156,7 @@ struct VncDisplay
>      int lock_key_sync;
>      QEMUPutLEDEntry *led;
>      int ledstate;
> -    int key_delay_ms;
> +    KbdState *kbd;
>      QemuMutex mutex;
>  
>      QEMUCursor *cursor;
> @@ -326,8 +327,6 @@ struct VncState
>  
>      VncReadEvent *read_handler;
>      size_t read_handler_expect;
> -    /* input */
> -    uint8_t modifiers_state[256];
>  
>      bool abort;
>      QemuMutex output_mutex;
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 0c1b477425..b56431ce3b 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c


> @@ -1797,32 +1780,20 @@ static void kbd_leds(void *opaque, int ledstate)
>  
>  static void do_key_event(VncState *vs, int down, int keycode, int sym)
>  {
> +    QKeyCode qcode = qemu_input_key_number_to_qcode(keycode);
> +
>      /* QEMU console switch */
> -    switch(keycode) {
> -    case 0x2a:                          /* Left Shift */
> -    case 0x36:                          /* Right Shift */
> -    case 0x1d:                          /* Left CTRL */
> -    case 0x9d:                          /* Right CTRL */
> -    case 0x38:                          /* Left ALT */
> -    case 0xb8:                          /* Right ALT */
> -        if (down)
> -            vs->modifiers_state[keycode] = 1;
> -        else
> -            vs->modifiers_state[keycode] = 0;
> -        break;


This code updated modifier state as the first thing in do_key_event


[snip]

> @@ -1859,30 +1828,25 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
>             toggles capslock away from the VNC window.
>          */
>          int uppercase = !!(sym >= 'A' && sym <= 'Z');
> -        int shift = !!(vs->modifiers_state[0x2a] | vs->modifiers_state[0x36]);
> -        int capslock = !!(vs->modifiers_state[0x3a]);
> +        bool shift = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_SHIFT);
> +        bool capslock = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CAPSLOCK);

This uses the modifier state

In old code it would use the newly updated modifier state

In new code it now uses the old modifier state.

>          if (capslock) {
>              if (uppercase == shift) {
>                  trace_vnc_key_sync_capslock(false);
> -                vs->modifiers_state[0x3a] = 0;
> -                press_key(vs, 0xffe5);
> +                press_key(vs, Q_KEY_CODE_CAPS_LOCK);
>              }
>          } else {
>              if (uppercase != shift) {
>                  trace_vnc_key_sync_capslock(true);
> -                vs->modifiers_state[0x3a] = 1;
> -                press_key(vs, 0xffe5);
> +                press_key(vs, Q_KEY_CODE_CAPS_LOCK);
>              }
>          }
>      }
>  
> -    if (qemu_console_is_graphic(NULL)) {
> -        qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, down);
> -        qemu_input_event_send_key_delay(vs->vd->key_delay_ms);
> -    } else {
> -        bool numlock = vs->modifiers_state[0x45];
> -        bool control = (vs->modifiers_state[0x1d] ||
> -                        vs->modifiers_state[0x9d]);
> +    kbd_state_key_event(vs->vd->kbd, qcode, down);

This updates modifier state in the new code, after the code above that
uses modifier state.

Is this ordering change intentional ?


> +    if (!qemu_console_is_graphic(NULL)) {
> +        bool numlock = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_NUMLOCK);
> +        bool control = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CTRL);
>          /* QEMU console emulation */
>          if (down) {
>              switch (keycode) {
> @@ -1983,27 +1947,6 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
>      }
>  }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH v2 6/7] keymap: pass full keyboard state to keysym2scancode

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 01:09:03PM +0100, Gerd Hoffmann wrote:
> Pass the keyboard state tracker handle down to keysym2scancode(),
> so the code can fully inspect the keyboard state as needed.  No
> functional change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/keymaps.h | 3 ++-
>  ui/curses.c  | 2 +-
>  ui/keymaps.c | 8 ++++----
>  ui/vnc.c     | 5 +----
>  4 files changed, 8 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH v2 7/7] keymap: fix keyup mappings

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 01:09:04PM +0100, Gerd Hoffmann wrote:
> It is possible that the modifier state on keyup is different from the
> modifier state on keydown.  In that case the keycode lookup can end up
> with different keys in case multiple keysym -> keycode mappings exist,
> because it picks the mapping depending on modifier state.
> 
> To fix that change the lookup logic for keyup events.  Instead of
> looking at the modifier state check the key state and prefer a keycodes
> where the key is in "down" state right now.
> 
> Fixes: abb4f2c965 keymap: consider modifier state when picking a mapping
> Buglink: https://bugs.launchpad.net/bugs/1738283
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1658676
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/keymaps.h |  2 +-
>  ui/curses.c  |  2 +-
>  ui/keymaps.c | 55 ++++++++++++++++++++++++++++++++++---------------------
>  ui/vnc.c     |  2 +-
>  4 files changed, 37 insertions(+), 24 deletions(-)


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH v2 0/7] ui: add generic keyboard state tracker, fix keymap

[no-reply]

Patchew URL: https://patchew.org/QEMU/20181219120904.17643-1-kraxel@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

libpmem support   no
libudev           no

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled:  'cc'
  GEN     x86_64-softmmu/config-devices.mak.tmp
---
  CC      chardev/char.o
  CC      chardev/char-fd.o
/tmp/qemu-test/src/ui/sdl.c: In function 'sdl_keyevent_to_keycode_generic':
/tmp/qemu-test/src/ui/sdl.c:218:28: error: incompatible type for argument 3 of 'keysym2scancode'
                            shift, altgr, ctrl) & SCANCODE_KEYMASK;
                            ^
In file included from /tmp/qemu-test/src/ui/sdl_keysym.h:2:0,
---
/tmp/qemu-test/src/ui/keymaps.h:58:5: note: expected 'struct KbdState *' but argument is of type '_Bool'
 int keysym2scancode(kbd_layout_t *k, int keysym,
     ^
/tmp/qemu-test/src/ui/sdl.c:218:28: error: too many arguments to function 'keysym2scancode'
                            shift, altgr, ctrl) & SCANCODE_KEYMASK;
                            ^
In file included from /tmp/qemu-test/src/ui/sdl_keysym.h:2:0,


The full log is available at
http://patchew.org/logs/20181219120904.17643-1-kraxel@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [RFC PATCH v2 5/7] kbd-state: use state tracker for vnc

[Gerd Hoffmann]

> > @@ -1859,30 +1828,25 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
> >             toggles capslock away from the VNC window.
> >          */
> >          int uppercase = !!(sym >= 'A' && sym <= 'Z');
> > -        int shift = !!(vs->modifiers_state[0x2a] | vs->modifiers_state[0x36]);
> > -        int capslock = !!(vs->modifiers_state[0x3a]);
> > +        bool shift = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_SHIFT);
> > +        bool capslock = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CAPSLOCK);
> 
> This uses the modifier state
> 
> In old code it would use the newly updated modifier state
> 
> In new code it now uses the old modifier state.

Ordering doesn't matter.  This code doesn't run on modifier key events.
On letter keydown events it checks whenever shift and capslock state are
consistent, and if not it generates a capslock keypress.

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH v2 5/7] kbd-state: use state tracker for vnc

[Daniel P. Berrangé]

On Tue, Jan 22, 2019 at 10:00:36AM +0100, Gerd Hoffmann wrote:
> > > @@ -1859,30 +1828,25 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
> > >             toggles capslock away from the VNC window.
> > >          */
> > >          int uppercase = !!(sym >= 'A' && sym <= 'Z');
> > > -        int shift = !!(vs->modifiers_state[0x2a] | vs->modifiers_state[0x36]);
> > > -        int capslock = !!(vs->modifiers_state[0x3a]);
> > > +        bool shift = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_SHIFT);
> > > +        bool capslock = kbd_state_modifier_get(vs->vd->kbd, KBD_MOD_CAPSLOCK);
> > 
> > This uses the modifier state
> > 
> > In old code it would use the newly updated modifier state
> > 
> > In new code it now uses the old modifier state.
> 
> Ordering doesn't matter.  This code doesn't run on modifier key events.
> On letter keydown events it checks whenever shift and capslock state are
> consistent, and if not it generates a capslock keypress.

Ok, then

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH v2 1/7] kbd-state: add keyboard state tracker

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]

On 12/19/18 6:08 AM, Gerd Hoffmann wrote:
> Now that most user interfaces are using QKeyCodes it is easier to have
> common keyboard code useable by all user interfaces.
> 
> This patch adds helper code to track the state of all keyboard keys,
> using a bitmap indexed by QKeyCode.  Modifier state is tracked too,
> as separate bitmap.  That makes checking modifier state easier.
> Likewise we can easily apply special handling for capslock & numlock
> (toggles on keypress) and ctrl + shift (we have two keys for that).
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/ui/kbd-state.h |  32 +++++++++++++
>  ui/kbd-state.c         | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
>  ui/Makefile.objs       |   2 +-
>  3 files changed, 158 insertions(+), 1 deletion(-)
>  create mode 100644 include/ui/kbd-state.h
>  create mode 100644 ui/kbd-state.c
> 

> +
> +void kbd_state_key_event(KbdState *kbd, QKeyCode qcode, bool down)
> +{
> +    bool state = test_bit(qcode, kbd->keys);
> +
> +    if (state == down) {
> +        /*
> +         * Filter out events which don't change the keyboard state.
> +         *
> +         * Most notably this allows to simply send along all key-up
> +         * events, and this function will filter out everything where
> +         * the corresponding key-down event wasn't send to the guest,

s/send/sent/

> +         * for example due to being a host hotkey.
> +         */
> +        return;

> +void kbd_state_lift_all_keys(KbdState *kbd)
> +{
> +    int qcode;
> +
> +    for (qcode = 0; qcode < Q_KEY_CODE__MAX; qcode++) {
> +        if (test_bit(qcode, kbd->keys)) {
> +            kbd_state_key_event(kbd, qcode, false);

Is there a more efficient iteration through the bitmap when looking for
the next set bit, or is the map small enough that it doesn't matter?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 2/7] kbd-state: use state tracker for sdl2

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On 12/19/18 6:08 AM, Gerd Hoffmann wrote:
> Use the new keyboard state tracked for sdl2.  We can drop the modifier

s/tracked/tracker/ ?

> state tracking from sdl2.  Also keyup code is simpler, the state tracker
> will take care to not send suspious keyup events to the guest.

s/suspious/suspicious/

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/ui/sdl2.h |  2 ++
>  ui/sdl2-input.c   | 40 +++-------------------------------------
>  ui/sdl2.c         | 12 +++---------
>  3 files changed, 8 insertions(+), 46 deletions(-)
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 1/7] kbd-state: add keyboard state tracker

[Gerd Hoffmann]

> > +         * the corresponding key-down event wasn't send to the guest,
> 
> s/send/sent/

Fixed.

> > +void kbd_state_lift_all_keys(KbdState *kbd)
> > +{
> > +    int qcode;
> > +
> > +    for (qcode = 0; qcode < Q_KEY_CODE__MAX; qcode++) {
> > +        if (test_bit(qcode, kbd->keys)) {
> > +            kbd_state_key_event(kbd, qcode, false);
> 
> Is there a more efficient iteration through the bitmap when looking for
> the next set bit, or is the map small enough that it doesn't matter?

It isn't that big (a bit over 100 I think), and the function isn't
called often, so performance really shouldn't be a problem here.

cheers,
  Gerd