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

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

[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

[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

[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 
> Reviewed-by: Daniel P. Berrangé 
> ---
>  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 & 0x) & 
> SCANCODE_KEYMASK;
> +keycode = keysym2scancode(vs->vd->kbd_layout, lsym & 0x,
> +  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)
 up0x3c(dot)
 up0xffe1  (shift)

Or

 down  0xffe1  (shift)
 down  0x3c(dot)
 up0xffe1  (shift)
 up0x3c(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 

[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 
Reviewed-by: Daniel P. Berrangé 
---
 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 =