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 <address@hidden>
> Reviewed-by: Daniel P. Berrangé <address@hidden>
> ---
>  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:

  address@hidden:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
  address@hidden:vnc_key_event_map down 1, sym 0x3c -> keycode 0x56 [less]
  address@hidden:vnc_key_event_map down 0, sym 0x3c -> keycode 0x56 [less]
  address@hidden:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]

  address@hidden:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
  address@hidden:vnc_key_event_map down 1, sym 0x3c -> keycode 0x56 [less]
  address@hidden:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]
  address@hidden: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:

  address@hidden:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
  address@hidden:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma]
  address@hidden:vnc_key_event_map down 0, sym 0x3c -> keycode 0x33 [comma]
  address@hidden:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]

  address@hidden:vnc_key_event_map down 1, sym 0xffe1 -> keycode 0x2a [shift]
  address@hidden:vnc_key_event_map down 1, sym 0x3c -> keycode 0x33 [comma]
  address@hidden:vnc_key_event_map down 0, sym 0xffe1 -> keycode 0x2a [shift]
  address@hidden: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 :|