[Qemu-devel] [PATCH v3 0/5] keymap: support kbd layouts with multiple mappings for the same key.

[Qemu-devel] [PATCH v3 0/5] keymap: support kbd layouts with multiple mappings for the same key.

[Gerd Hoffmann]

The reverse keymap (-k $map) code has problems dealing with keymaps
where multiple key combinations can create the same keysym, because
it can store a single keycode per keysym only.  This series fixes it
and does some cleanups along the way.

v2: rebase, codestyle fixes.
v3: use GINT_TO_POINTER

Gerd Hoffmann (5):
  keymap: make struct kbd_layout_t private to ui/keymaps.c
  keymap: use glib hash for kbd_layout_t
  keymap: numpad keysyms and keycodes are fixed
  keymap: record multiple keysym -> keycode mappings
  keymap: consider modifier state when picking a mapping

 ui/keymaps.h    |  30 +++--------
 ui/curses.c     |   3 +-
 ui/keymaps.c    | 163 +++++++++++++++++++++++++++++---------------------------
 ui/sdl.c        |   6 ++-
 ui/vnc.c        |   9 +++-
 ui/trace-events |   2 +-
 6 files changed, 105 insertions(+), 108 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v3 1/5] keymap: make struct kbd_layout_t private to ui/keymaps.c

[Gerd Hoffmann]

Also use kbd_layout_t pointers instead of void pointers.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/keymaps.h | 29 ++++++-----------------------
 ui/keymaps.c | 32 +++++++++++++++++++++++++-------
 2 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/ui/keymaps.h b/ui/keymaps.h
index 8757465529..17ec03387a 100644
--- a/ui/keymaps.h
+++ b/ui/keymaps.h
@@ -32,25 +32,6 @@ typedef struct {
 	int keysym;
 } name2keysym_t;
 
-struct key_range {
-    int start;
-    int end;
-    struct key_range *next;
-};
-
-#define MAX_NORMAL_KEYCODE 512
-#define MAX_EXTRA_COUNT 256
-typedef struct {
-    uint16_t keysym2keycode[MAX_NORMAL_KEYCODE];
-    struct {
-	int keysym;
-	uint16_t keycode;
-    } keysym2keycode_extra[MAX_EXTRA_COUNT];
-    int extra_count;
-    struct key_range *keypad_range;
-    struct key_range *numlock_range;
-} kbd_layout_t;
-
 /* scancode without modifiers */
 #define SCANCODE_KEYMASK 0xff
 /* scancode without grey or up bit */
@@ -69,10 +50,12 @@ typedef struct {
 #define SCANCODE_ALT    0x400
 #define SCANCODE_ALTGR  0x800
 
+typedef struct kbd_layout_t kbd_layout_t;
 
-void *init_keyboard_layout(const name2keysym_t *table, const char *language);
-int keysym2scancode(void *kbd_layout, int keysym);
-int keycode_is_keypad(void *kbd_layout, int keycode);
-int keysym_is_numlock(void *kbd_layout, int keysym);
+kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
+                                   const char *language);
+int keysym2scancode(kbd_layout_t *k, int keysym);
+int keycode_is_keypad(kbd_layout_t *k, int keycode);
+int keysym_is_numlock(kbd_layout_t *k, int keysym);
 
 #endif /* QEMU_KEYMAPS_H */
diff --git a/ui/keymaps.c b/ui/keymaps.c
index f9762d1497..134958a197 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -28,6 +28,26 @@
 #include "trace.h"
 #include "qemu/error-report.h"
 
+#define MAX_NORMAL_KEYCODE 512
+#define MAX_EXTRA_COUNT 256
+
+struct key_range {
+    int start;
+    int end;
+    struct key_range *next;
+};
+
+struct kbd_layout_t {
+    uint16_t keysym2keycode[MAX_NORMAL_KEYCODE];
+    struct {
+        int keysym;
+        uint16_t keycode;
+    } keysym2keycode_extra[MAX_EXTRA_COUNT];
+    int extra_count;
+    struct key_range *keypad_range;
+    struct key_range *numlock_range;
+};
+
 static int get_keysym(const name2keysym_t *table,
                       const char *name)
 {
@@ -186,15 +206,15 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
 }
 
 
-void *init_keyboard_layout(const name2keysym_t *table, const char *language)
+kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
+                                   const char *language)
 {
     return parse_keyboard_layout(table, language, NULL);
 }
 
 
-int keysym2scancode(void *kbd_layout, int keysym)
+int keysym2scancode(kbd_layout_t *k, int keysym)
 {
-    kbd_layout_t *k = kbd_layout;
     if (keysym < MAX_NORMAL_KEYCODE) {
         if (k->keysym2keycode[keysym] == 0) {
             trace_keymap_unmapped(keysym);
@@ -217,9 +237,8 @@ int keysym2scancode(void *kbd_layout, int keysym)
     return 0;
 }
 
-int keycode_is_keypad(void *kbd_layout, int keycode)
+int keycode_is_keypad(kbd_layout_t *k, int keycode)
 {
-    kbd_layout_t *k = kbd_layout;
     struct key_range *kr;
 
     for (kr = k->keypad_range; kr; kr = kr->next) {
@@ -230,9 +249,8 @@ int keycode_is_keypad(void *kbd_layout, int keycode)
     return 0;
 }
 
-int keysym_is_numlock(void *kbd_layout, int keysym)
+int keysym_is_numlock(kbd_layout_t *k, int keysym)
 {
-    kbd_layout_t *k = kbd_layout;
     struct key_range *kr;
 
     for (kr = k->numlock_range; kr; kr = kr->next) {
-- 
2.9.3

[Qemu-devel] [PATCH v3 2/5] keymap: use glib hash for kbd_layout_t

[Gerd Hoffmann]

Drop home-grown lookup code, which is a strange mix of a lookup table
and a list.  Use standard glib hash instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/keymaps.c    | 73 ++++++++++++++++++++++++---------------------------------
 ui/trace-events |  2 +-
 2 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/ui/keymaps.c b/ui/keymaps.c
index 134958a197..449c3dec22 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -28,22 +28,18 @@
 #include "trace.h"
 #include "qemu/error-report.h"
 
-#define MAX_NORMAL_KEYCODE 512
-#define MAX_EXTRA_COUNT 256
-
 struct key_range {
     int start;
     int end;
     struct key_range *next;
 };
 
+struct keysym2code {
+    uint16_t keycode;
+};
+
 struct kbd_layout_t {
-    uint16_t keysym2keycode[MAX_NORMAL_KEYCODE];
-    struct {
-        int keysym;
-        uint16_t keycode;
-    } keysym2keycode_extra[MAX_EXTRA_COUNT];
-    int extra_count;
+    GHashTable *hash;
     struct key_range *keypad_range;
     struct key_range *numlock_range;
 };
@@ -91,23 +87,19 @@ static void add_to_key_range(struct key_range **krp, int code) {
     }
 }
 
-static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k) {
-    if (keysym < MAX_NORMAL_KEYCODE) {
-        trace_keymap_add("normal", keysym, keycode, line);
-        k->keysym2keycode[keysym] = keycode;
-    } else {
-        if (k->extra_count >= MAX_EXTRA_COUNT) {
-            warn_report("Could not assign keysym %s (0x%x)"
-                        " because of memory constraints.", line, keysym);
-        } else {
-            trace_keymap_add("extra", keysym, keycode, line);
-            k->keysym2keycode_extra[k->extra_count].
-            keysym = keysym;
-            k->keysym2keycode_extra[k->extra_count].
-            keycode = keycode;
-            k->extra_count++;
-        }
+static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
+{
+    struct keysym2code *keysym2code;
+
+    keysym2code = g_hash_table_lookup(k->hash, GINT_TO_POINTER(keysym));
+    if (keysym2code) {
+        return;
     }
+
+    keysym2code = g_new0(struct keysym2code, 1);
+    keysym2code->keycode = keycode;
+    g_hash_table_replace(k->hash, GINT_TO_POINTER(keysym), keysym2code);
+    trace_keymap_add(keysym, keycode, line);
 }
 
 static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
@@ -131,6 +123,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
 
     if (!k) {
         k = g_new0(kbd_layout_t, 1);
+        k->hash = g_hash_table_new(NULL, NULL);
     }
 
     for(;;) {
@@ -215,26 +208,22 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
 
 int keysym2scancode(kbd_layout_t *k, int keysym)
 {
-    if (keysym < MAX_NORMAL_KEYCODE) {
-        if (k->keysym2keycode[keysym] == 0) {
-            trace_keymap_unmapped(keysym);
-            warn_report("no scancode found for keysym %d", keysym);
-        }
-        return k->keysym2keycode[keysym];
-    } else {
-        int i;
+    struct keysym2code *keysym2code;
+
 #ifdef XK_ISO_Left_Tab
-        if (keysym == XK_ISO_Left_Tab) {
-            keysym = XK_Tab;
-        }
+    if (keysym == XK_ISO_Left_Tab) {
+        keysym = XK_Tab;
+    }
 #endif
-        for (i = 0; i < k->extra_count; i++) {
-            if (k->keysym2keycode_extra[i].keysym == keysym) {
-                return k->keysym2keycode_extra[i].keycode;
-            }
-        }
+
+    keysym2code = g_hash_table_lookup(k->hash, GINT_TO_POINTER(keysym));
+    if (!keysym2code) {
+        trace_keymap_unmapped(keysym);
+        warn_report("no scancode found for keysym %d", keysym);
+        return 0;
     }
-    return 0;
+
+    return keysym2code->keycode;
 }
 
 int keycode_is_keypad(kbd_layout_t *k, int keycode)
diff --git a/ui/trace-events b/ui/trace-events
index 34229e6747..861b68a305 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -78,7 +78,7 @@ qemu_spice_create_update(uint32_t left, uint32_t right, uint32_t top, uint32_t b
 
 # ui/keymaps.c
 keymap_parse(const char *file) "file %s"
-keymap_add(const char *type, int sym, int code, const char *line) "%-6s sym=0x%04x code=0x%04x (line: %s)"
+keymap_add(int sym, int code, const char *line) "sym=0x%04x code=0x%04x (line: %s)"
 keymap_unmapped(int sym) "sym=0x%04x"
 
 # ui/x_keymap.c
-- 
2.9.3

[Qemu-devel] [PATCH v3 3/5] keymap: numpad keysyms and keycodes are fixed

[Gerd Hoffmann]

No need to figure them at runtime from the keymap.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/keymaps.c | 61 +++++++++---------------------------------------------------
 1 file changed, 9 insertions(+), 52 deletions(-)

diff --git a/ui/keymaps.c b/ui/keymaps.c
index 449c3dec22..cbdd65c767 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -28,20 +28,12 @@
 #include "trace.h"
 #include "qemu/error-report.h"
 
-struct key_range {
-    int start;
-    int end;
-    struct key_range *next;
-};
-
 struct keysym2code {
     uint16_t keycode;
 };
 
 struct kbd_layout_t {
     GHashTable *hash;
-    struct key_range *keypad_range;
-    struct key_range *numlock_range;
 };
 
 static int get_keysym(const name2keysym_t *table,
@@ -64,29 +56,6 @@ static int get_keysym(const name2keysym_t *table,
 }
 
 
-static void add_to_key_range(struct key_range **krp, int code) {
-    struct key_range *kr;
-    for (kr = *krp; kr; kr = kr->next) {
-        if (code >= kr->start && code <= kr->end) {
-            break;
-        }
-        if (code == kr->start - 1) {
-            kr->start--;
-            break;
-        }
-        if (code == kr->end + 1) {
-            kr->end++;
-            break;
-        }
-    }
-    if (kr == NULL) {
-        kr = g_malloc0(sizeof(*kr));
-        kr->start = kr->end = code;
-        kr->next = *krp;
-        *krp = kr;
-    }
-}
-
 static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
 {
     struct keysym2code *keysym2code;
@@ -160,13 +129,6 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
                     const char *rest = line + offset + 1;
                     int keycode = strtol(rest, NULL, 0);
 
-                    if (strstr(rest, "numlock")) {
-                        add_to_key_range(&k->keypad_range, keycode);
-                        add_to_key_range(&k->numlock_range, keysym);
-                        /* fprintf(stderr, "keypad keysym %04x keycode %d\n",
-                                   keysym, keycode); */
-                    }
-
                     if (strstr(rest, "shift")) {
                         keycode |= SCANCODE_SHIFT;
                     }
@@ -228,24 +190,19 @@ int keysym2scancode(kbd_layout_t *k, int keysym)
 
 int keycode_is_keypad(kbd_layout_t *k, int keycode)
 {
-    struct key_range *kr;
-
-    for (kr = k->keypad_range; kr; kr = kr->next) {
-        if (keycode >= kr->start && keycode <= kr->end) {
-            return 1;
-        }
+    if (keycode >= 0x47 && keycode <= 0x53) {
+        return true;
     }
-    return 0;
+    return false;
 }
 
 int keysym_is_numlock(kbd_layout_t *k, int keysym)
 {
-    struct key_range *kr;
-
-    for (kr = k->numlock_range; kr; kr = kr->next) {
-        if (keysym >= kr->start && keysym <= kr->end) {
-            return 1;
-        }
+    switch (keysym) {
+    case 0xffb0 ... 0xffb9:  /* KP_0 .. KP_9 */
+    case 0xffac:             /* KP_Separator */
+    case 0xffae:             /* KP_Decimal   */
+        return true;
     }
-    return 0;
+    return false;
 }
-- 
2.9.3

[Qemu-devel] [PATCH v3 4/5] keymap: record multiple keysym -> keycode mappings

[Gerd Hoffmann]

Sometimes the same keysym can be created using different key
combinations.  Record them all in the reverse keymap, not only
the first one.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/keymaps.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/ui/keymaps.c b/ui/keymaps.c
index cbdd65c767..1eba6d7057 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -29,7 +29,8 @@
 #include "qemu/error-report.h"
 
 struct keysym2code {
-    uint16_t keycode;
+    uint32_t count;
+    uint16_t keycodes[4];
 };
 
 struct kbd_layout_t {
@@ -62,11 +63,18 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
 
     keysym2code = g_hash_table_lookup(k->hash, GINT_TO_POINTER(keysym));
     if (keysym2code) {
+        if (keysym2code->count < ARRAY_SIZE(keysym2code->keycodes)) {
+            keysym2code->keycodes[keysym2code->count++] = keycode;
+        } else {
+            warn_report("more than %zd keycodes for keysym %d",
+                        ARRAY_SIZE(keysym2code->keycodes), keysym);
+        }
         return;
     }
 
     keysym2code = g_new0(struct keysym2code, 1);
-    keysym2code->keycode = keycode;
+    keysym2code->keycodes[0] = keycode;
+    keysym2code->count = 1;
     g_hash_table_replace(k->hash, GINT_TO_POINTER(keysym), keysym2code);
     trace_keymap_add(keysym, keycode, line);
 }
@@ -185,7 +193,7 @@ int keysym2scancode(kbd_layout_t *k, int keysym)
         return 0;
     }
 
-    return keysym2code->keycode;
+    return keysym2code->keycodes[0];
 }
 
 int keycode_is_keypad(kbd_layout_t *k, int keycode)
-- 
2.9.3

[Qemu-devel] [PATCH v3 5/5] keymap: consider modifier state when picking a mapping

[Gerd Hoffmann]

Pass the modifier state to the keymap lookup function.  In case multiple
keysym -> keycode mappings exist look at the modifier state and prefer
the mapping where the modifier state matches.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/keymaps.h |  3 ++-
 ui/curses.c  |  3 ++-
 ui/keymaps.c | 33 ++++++++++++++++++++++++++++++++-
 ui/sdl.c     |  6 +++++-
 ui/vnc.c     |  9 +++++++--
 5 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/ui/keymaps.h b/ui/keymaps.h
index 17ec03387a..0693588225 100644
--- a/ui/keymaps.h
+++ b/ui/keymaps.h
@@ -54,7 +54,8 @@ typedef struct kbd_layout_t kbd_layout_t;
 
 kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
                                    const char *language);
-int keysym2scancode(kbd_layout_t *k, int keysym);
+int keysym2scancode(kbd_layout_t *k, int keysym,
+                    bool shift, bool altgr, bool ctrl);
 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 479b77bd03..597e47fd4a 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -271,7 +271,8 @@ static void curses_refresh(DisplayChangeListener *dcl)
                     keysym = chr;
             }
 
-            keycode = keysym2scancode(kbd_layout, keysym & KEYSYM_MASK);
+            keycode = keysym2scancode(kbd_layout, keysym & KEYSYM_MASK,
+                                      false, false, false);
             if (keycode == 0)
                 continue;
 
diff --git a/ui/keymaps.c b/ui/keymaps.c
index 1eba6d7057..43fe604724 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -176,8 +176,12 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
 }
 
 
-int keysym2scancode(kbd_layout_t *k, int keysym)
+int keysym2scancode(kbd_layout_t *k, int keysym,
+                    bool shift, bool altgr, bool ctrl)
 {
+    static const uint32_t mask =
+        SCANCODE_SHIFT | SCANCODE_ALTGR | SCANCODE_CTRL;
+    uint32_t mods, i;
     struct keysym2code *keysym2code;
 
 #ifdef XK_ISO_Left_Tab
@@ -193,6 +197,33 @@ int keysym2scancode(kbd_layout_t *k, int keysym)
         return 0;
     }
 
+    if (keysym2code->count == 1) {
+        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 (shift) {
+        mods |= SCANCODE_SHIFT;
+    }
+    if (altgr) {
+        mods |= SCANCODE_ALTGR;
+    }
+    if (ctrl) {
+        mods |= SCANCODE_CTRL;
+    }
+
+    for (i = 0; i < keysym2code->count; i++) {
+        if ((keysym2code->keycodes[i] & mask) == mods) {
+            return keysym2code->keycodes[i];
+        }
+    }
     return keysym2code->keycodes[0];
 }
 
diff --git a/ui/sdl.c b/ui/sdl.c
index 963cdf77a7..c4ae7ab05d 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -201,6 +201,9 @@ static kbd_layout_t *kbd_layout = NULL;
 
 static uint8_t sdl_keyevent_to_keycode_generic(const SDL_KeyboardEvent *ev)
 {
+    bool shift = modifiers_state[0x2a] || modifiers_state[0x36];
+    bool altgr = modifiers_state[0xb8];
+    bool ctrl  = modifiers_state[0x1d] || modifiers_state[0x9d];
     int keysym;
     /* workaround for X11+SDL bug with AltGR */
     keysym = ev->keysym.sym;
@@ -210,7 +213,8 @@ static uint8_t sdl_keyevent_to_keycode_generic(const SDL_KeyboardEvent *ev)
     if (keysym == 92 && ev->keysym.scancode == 133) {
         keysym = 0xa5;
     }
-    return keysym2scancode(kbd_layout, keysym) & SCANCODE_KEYMASK;
+    return keysym2scancode(kbd_layout, keysym,
+                           shift, altgr, ctrl) & SCANCODE_KEYMASK;
 }
 
 
diff --git a/ui/vnc.c b/ui/vnc.c
index a77b568b57..d19f86c7f4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1734,7 +1734,8 @@ static void reset_keys(VncState *vs)
 
 static void press_key(VncState *vs, int keysym)
 {
-    int keycode = keysym2scancode(vs->vd->kbd_layout, keysym) & SCANCODE_KEYMASK;
+    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);
@@ -1993,6 +1994,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];
     int keycode;
     int lsym = sym;
 
@@ -2000,7 +2004,8 @@ static void key_event(VncState *vs, int down, uint32_t sym)
         lsym = lsym - 'A' + 'a';
     }
 
-    keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF) & SCANCODE_KEYMASK;
+    keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF,
+                              shift, altgr, ctrl) & 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] [PATCH v3 2/5] keymap: use glib hash for kbd_layout_t

[Daniel P. Berrangé]

On Thu, Feb 22, 2018 at 08:05:10AM +0100, Gerd Hoffmann wrote:
> Drop home-grown lookup code, which is a strange mix of a lookup table
> and a list.  Use standard glib hash instead.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/keymaps.c    | 73 ++++++++++++++++++++++++---------------------------------
>  ui/trace-events |  2 +-
>  2 files changed, 32 insertions(+), 43 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] [PATCH v3 5/5] keymap: consider modifier state when picking a mapping

[Daniel P. Berrangé]

On Thu, Feb 22, 2018 at 08:05:13AM +0100, Gerd Hoffmann wrote:
> Pass the modifier state to the keymap lookup function.  In case multiple
> keysym -> keycode mappings exist look at the modifier state and prefer
> the mapping where the modifier state matches.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  ui/keymaps.h |  3 ++-
>  ui/curses.c  |  3 ++-
>  ui/keymaps.c | 33 ++++++++++++++++++++++++++++++++-
>  ui/sdl.c     |  6 +++++-
>  ui/vnc.c     |  9 +++++++--
>  5 files changed, 48 insertions(+), 6 deletions(-)

[snip]

> diff --git a/ui/vnc.c b/ui/vnc.c
> index a77b568b57..d19f86c7f4 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1734,7 +1734,8 @@ static void reset_keys(VncState *vs)
>  
>  static void press_key(VncState *vs, int keysym)
>  {
> -    int keycode = keysym2scancode(vs->vd->kbd_layout, keysym) & SCANCODE_KEYMASK;
> +    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);
> @@ -1993,6 +1994,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];
>      int keycode;
>      int lsym = sym;
>  
> @@ -2000,7 +2004,8 @@ static void key_event(VncState *vs, int down, uint32_t sym)
>          lsym = lsym - 'A' + 'a';
>      }
>  
> -    keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF) & SCANCODE_KEYMASK;
> +    keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0xFFFF,
> +                              shift, altgr, ctrl) & SCANCODE_KEYMASK;
>      trace_vnc_key_event_map(down, sym, keycode, code2name(keycode));
>      do_key_event(vs, down, keycode, sym);
>  }

It looks like this patch is causing some buggy input handling behaviour
with VNC (and probably SDL though I didn't test)


Consider the user wants to type a "<" character.  There are two valid
key sequences for this

 down  0xffe1      (shift)
 down  0x3c        (dot)
 up    0x3c        (dot)
 up    0xffe1      (shift)

Or

 down  0xffe1      (shift)
 down  0x3c        (dot)
 up    0xffe1      (shift)
 up    0x3c        (dot)


With the original code, the "dot" character would be intepreted the
same way in both sequences.

With this patch applied, the "dot", the second sequence is broken.
The "dot" character is interpreted with no modifiers on down, and
interpreted with the shift modifier on release. This is then causing
QEMU to see a different QKeyCode on down vs up.

The trace events show the problem:

Before this patch, the two sequences show:

  18544@1545157940.945319:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
  18544@1545157941.186608:vnc_key_event_map down 1, sym 0x3c -> keycode 0x56 [less]
  18544@1545157941.346898:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less]
  18544@1545157941.810041:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]

  18544@1545157943.447085:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
  18544@1545157943.752193:vnc_key_event_map down 1, sym 0x3c -> keycode 0x56 [less]
  18544@1545157943.986145:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]
  18544@1545157944.258757:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less]

IOW, in both sequences we have matched pairs of events


After this patch the two sequences now show:

  19237@1545158356.679095:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
  19237@1545158356.896528:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma]
  19237@1545158356.993672:vnc_key_event_map down 0, sym 0x3c -> keycode 0x33 [comma]
  19237@1545158357.233437:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]

  19237@1545158358.726002:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
  19237@1545158358.999542:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma]
  19237@1545158359.201503:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]
  19237@1545158359.435021:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less]

Notice how we sent a "down(comma)" and paired it with an "up(less)". This
causes the guest to consider the key stuck down, so next time you type
a "<" it will get lost.

This is easily reproduced with gtk-vnc and the following QEMU command
line:

  ./x86_64-softmmu/qemu-system-x86_64 -vnc :1 -d trace:vnc* 


I don't see any nice way to fix the problem without having to keep a
memory of modifier state with every "down" event, so you can use the
same modifier state with the later "up" event to ensure you get a
matching key up.  This is quite horrible though. I'm more inclined
to revert this patch and find another way to fix the original problem
which won't require the UI frontends to track modifier state.

This problem is reported in

  https://bugs.launchpad.net/bugs/1738283
  https://bugzilla.redhat.com/show_bug.cgi?id=1658676

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] [PATCH v3 5/5] keymap: consider modifier state when picking a mapping

[Gerd Hoffmann]

  Hi,

> After this patch the two sequences now show:
> 
>   19237@1545158356.679095:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
>   19237@1545158356.896528:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma]
>   19237@1545158356.993672:vnc_key_event_map down 0, sym 0x3c -> keycode 0x33 [comma]
>   19237@1545158357.233437:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]
> 
>   19237@1545158358.726002:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
>   19237@1545158358.999542:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma]
>   19237@1545158359.201503:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]
>   19237@1545158359.435021:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less]
> 
> Notice how we sent a "down(comma)" and paired it with an "up(less)". This
> causes the guest to consider the key stuck down, so next time you type
> a "<" it will get lost.

Ouch.

> This is easily reproduced with gtk-vnc and the following QEMU command
> line:
> 
>   ./x86_64-softmmu/qemu-system-x86_64 -vnc :1 -d trace:vnc* 
> 
> 
> I don't see any nice way to fix the problem without having to keep a
> memory of modifier state with every "down" event, so you can use the
> same modifier state with the later "up" event to ensure you get a
> matching key up.

Well, for key-up you can prefer keys in "down" state instead of looking
at the modifiers.

> This is quite horrible though. I'm more inclined
> to revert this patch and find another way to fix the original problem
> which won't require the UI frontends to track modifier state.

The UIs track modifier state anyway.

I fact I have some WIP patches to add a generic keyboard state tracker,
so the UIs can use common code instead of having their own state
tracking code each.  Also to make UI hotkeys configurable, consistent
across all UIs we have.  Guess I should undust them, at least the state
tracking part of it.

With that in place we can easily pass the full keyboard state to the
keymap code.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 5/5] keymap: consider modifier state when picking a mapping

[Gerd Hoffmann]

  Hi,
 
> > This is quite horrible though. I'm more inclined
> > to revert this patch and find another way to fix the original problem
> > which won't require the UI frontends to track modifier state.
> 
> The UIs track modifier state anyway.
> 
> I fact I have some WIP patches to add a generic keyboard state tracker,
> so the UIs can use common code instead of having their own state
> tracking code each.  Also to make UI hotkeys configurable, consistent
> across all UIs we have.  Guess I should undust them, at least the state
> tracking part of it.
> 
> With that in place we can easily pass the full keyboard state to the
> keymap code.

Tried that:
  https://git.kraxel.org/cgit/qemu/log/?h=sirius/kbd-state

Not polished yet.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 5/5] keymap: consider modifier state when picking a mapping

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 10:42:14AM +0100, Gerd Hoffmann wrote:
>   Hi,
>  
> > > This is quite horrible though. I'm more inclined
> > > to revert this patch and find another way to fix the original problem
> > > which won't require the UI frontends to track modifier state.
> > 
> > The UIs track modifier state anyway.
> > 
> > I fact I have some WIP patches to add a generic keyboard state tracker,
> > so the UIs can use common code instead of having their own state
> > tracking code each.  Also to make UI hotkeys configurable, consistent
> > across all UIs we have.  Guess I should undust them, at least the state
> > tracking part of it.
> > 
> > With that in place we can easily pass the full keyboard state to the
> > keymap code.
> 
> Tried that:
>   https://git.kraxel.org/cgit/qemu/log/?h=sirius/kbd-state

I've had a look & think that code makes sense & ought to be able to
solve this problem.


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] [PATCH v3 5/5] keymap: consider modifier state when picking a mapping

[Adam Williamson]

On Wed, 2018-12-19 at 11:17 +0000, Daniel P. Berrangé wrote:
> On Wed, Dec 19, 2018 at 10:42:14AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> >  
> > > > This is quite horrible though. I'm more inclined
> > > > to revert this patch and find another way to fix the original problem
> > > > which won't require the UI frontends to track modifier state.
> > > 
> > > The UIs track modifier state anyway.
> > > 
> > > I fact I have some WIP patches to add a generic keyboard state tracker,
> > > so the UIs can use common code instead of having their own state
> > > tracking code each.  Also to make UI hotkeys configurable, consistent
> > > across all UIs we have.  Guess I should undust them, at least the state
> > > tracking part of it.
> > > 
> > > With that in place we can easily pass the full keyboard state to the
> > > keymap code.
> > 
> > Tried that:
> >   https://git.kraxel.org/cgit/qemu/log/?h=sirius/kbd-state
> 
> I've had a look & think that code makes sense & ought to be able to
> solve this problem.

I'll try and find a minute to do a scratch build with this patch (and
without my keymap patch) and throw it on openQA staging, since openQA
seems to do a great job of testing qemu input code. :P
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net