Re: [Qemu-devel] [PATCH 02/15] ui: convert common input code to keycodemapdb

From: Eric Blake <eblake@redhat.com>
To: "Daniel P. Berrange" <berrange@redhat.com>, qemu-devel@nongnu.org
Cc: Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 02/15] ui: convert common input code to keycodemapdb
Date: Thu, 10 Aug 2017 13:59:09 -0500	[thread overview]
Message-ID: <5e2768ee-17e6-b33d-2787-ab9fb940615f@redhat.com> (raw)
In-Reply-To: <20170810155522.31099-3-berrange@redhat.com>

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

On 08/10/2017 10:55 AM, Daniel P. Berrange wrote:
> Replace the number_to_qcode, qcode_to_number and linux_to_qcode
> tables with automatically generated tables.
> 
> Missing entries in linux_to_qcode now fixed:

> In additionsome fixes:

s/additionsome/addition, some/

> 
>  - KEY_PLAYPAUSE now maps to Q_KEY_CODE_AUDIOPLAY, instead of
>    KEY_PLAYCD. KEY_PLAYPAUSE is defined across almost all scancodes
>    sets, while KEY_PLAYCD only appears in AT set1, so the former is
>    a more useful mapping.
> 
> Missing entries in qcode_to_number now fixed:
> 
>   Q_KEY_CODE_AGAIN -> 0x85

I didn't research that these mappings are correct in relation to an
official documentation, but trust that you have done due diligence.

> In addition some fixes:
> 
>  - Q_KEY_CODE_MENU was incorrectly mapped to the compose
>    scancode (0xdd) and is now mapped to 0x9e
>  - Q_KEY_CODE_FIND was mapped to 0xe065 (Search) instead
>    of to 0xe041 (Find)
>  - Q_KEY_CODE_HIRAGANA was mapped to 0x70 (Katakanahiragana)
>    instead of of 0x77 (Hirigana)
>  - Q_KEY_CODE_PRINT was mapped to 0xb7 which is not a defined
>    scan code in AT set 1, it is now mapped to 0x54 (sysrq)
> 

Are any of these fixes something we need in 2.10 (more likely, as manual
fixes rather than via the git submodule)?  At this point, though, I'm
inclined to say we're deep enough in freeze that if it is not a
regression over 2.9 behavior, it's not worth rushing in the fix to 2.10.

> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/ui/input.h |  11 +-
>  ui/Makefile.objs   |   3 +
>  ui/input-keymap.c  | 326 +++--------------------------------------------------
>  3 files changed, 28 insertions(+), 312 deletions(-)
> 
> +++ b/ui/Makefile.objs
> @@ -54,6 +54,9 @@ KEYCODEMAP_GEN = ui/keycodemapdb/tools/keymap-gen
>  KEYCODEMAP_CSV = ui/keycodemapdb/data/keymaps.csv
>  
>  KEYCODEMAP_FILES = \
> +		 ui/input-keymap-linux2qcode.c \
> +		 ui/input-keymap-qcode2qnum.c \
> +		 ui/input-keymap-qnum2qcode.c \

My comment on patch 1 complained about regex of [a-zA-Z0-9] - do any of
the keycode names have digits, or can you shorten the regex to [a-zA-Z]?

> -
> -static int number_to_qcode[0x100];
> +#include "ui/input-keymap-linux2qcode.c"
> +#include "ui/input-keymap-qcode2qnum.c"
> +#include "ui/input-keymap-qnum2qcode.c"
>  
>  int qemu_input_linux_to_qcode(unsigned int lnx)
>  {
> -    assert(lnx < KEY_CNT);
> -    return linux_to_qcode[lnx];

The old code asserted on an out-of-range input,

> +    if (lnx >= qemu_input_map_linux2qcode_len) {
> +        return 0;
> +    }
> +    return qemu_input_map_linux2qcode[lnx];

the new code returns 0.  I guess that's okay, though, since the
generated table uses 0 for invalid entries, and there's no implicit
reason why out-of-range input has to assert.

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

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