Re: [Qemu-devel] [PATCH 1/2] Rewrite the way keycode conversions are performed

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/18/2012 12:31 PM, Daniel P. Berrange wrote:
> On Wed, Jan 18, 2012 at 11:47:12AM -0600, Anthony Liguori wrote:
>> On 01/18/2012 11:07 AM, Daniel P. Berrange wrote:
>>> On Wed, Jan 18, 2012 at 10:53:57AM -0600, Anthony Liguori wrote:
>>>> On 01/17/2012 01:35 PM, Daniel P. Berrange wrote:
>>>>> From: "Daniel P. Berrange"<berrange@redhat.com>
>>>>>
>>>>> The SDL video display code needs to convert between the SDL keyboard
>>>>> event keycodes, and QEMU's internal keycode set (a variant of the
>>>>> xt coding).  Currently the SDL code is only able todo this when it is
>>>>> built for X11, and running against a Linux X11 server using evdev or
>>>>> xfree86 keymaps.  If running against an OS-X or Win32 X11 server the
>>>>> keycode mapping falls back to xfre86, which is completely wrong. There
>>>>> is no mapping at all done, if built against an SDL library with direct
>>>>> Win32 or Quartz display support.
>>>>>
>>>>> After initial creation, this QEMU code was later copied into GTK-VNC
>>>>> to deal with the same problem. GTK-VNC came across the same problem
>>>>> described above and rewrote the mapping code from scratch. Instead
>>>>> of creating two arrays for the specific conversions required, the
>>>>> GTK-VNC code created a CSV file containing data for all commonly
>>>>> known keycode sets (Linux evdev, OS-X, AT set1, AT set2, AT set3,
>>>>> XT, XT Linux KBD driver, USB HID, Win32, Xwin XT variant, Xorg
>>>>> KBD XT variant). A script is then used to generate C arrays for
>>>>> the particular conversions required.  The CSV file has since been
>>>>> reused in both the SPICE-GTK and libvirt codebases, unchanged.
>>>>>
>>>>> This patch rewrites QEMU's SDL code to use this same approach, and
>>>>> in the process adds support for 4 new targets, SDL X11 on Win32
>>>>> Xorg server, SDL X11 on OS-X Xorg server, SDL Win32 and SDL Quartz.
>>>>>
>>>>> In the near future the 'keymap-gen.pl' and 'keymaps.csv' files
>>>>> will be placed into a dedicated GIT repo which can be added to
>>>>> QEMU, libvirt, SPICE-GTK&    GTK-VNC via a git submodule, instead
>>>>> of requiring manual copying.
>>>>>
>>>>> * Makefile: Add rules for generating keymap data files
>>>>> * Makefile.objs: Replace x_keymap.o with sdl_keymap.o
>>>>> * ui/keymap-gen.pl: Script for generating keycode mapping tables
>>>>> * ui/keymaps.csv: Master datafile of keycode mappings
>>>>> * ui/sdl.c: Rewrite sdl_keyevent_to_keycode to use new
>>>>>    mapping code
>>>>> * ui/sdl_keymap.c,ui/sdl_keymap.h: Add APIs for detecting
>>>>>    suitable keymap tables&    performing keymap conversions
>>>>> * ui/x_keymap.[ch]: Remove obsolete files
>>>>> ---
>>>>>   Makefile         |   40 +++++-
>>>>>   Makefile.objs    |    2 +-
>>>>>   ui/keymap-gen.pl |  210 ++++++++++++++++++++++++
>>>>>   ui/keymaps.csv   |  464 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   ui/sdl.c         |   88 ++---------
>>>>>   ui/sdl_keymap.c  |  241 ++++++++++++++++++++++++++++
>>>>>   ui/sdl_keymap.h  |   32 ++++
>>>>>   ui/x_keymap.c    |  168 --------------------
>>>>>   ui/x_keymap.h    |   32 ----
>>>>>   9 files changed, 998 insertions(+), 279 deletions(-)
>>>>>   create mode 100644 ui/keymap-gen.pl
>>>>>   create mode 100644 ui/keymaps.csv
>>>>>   create mode 100644 ui/sdl_keymap.c
>>>>>   create mode 100644 ui/sdl_keymap.h
>>>>>   delete mode 100644 ui/x_keymap.c
>>>>>   delete mode 100644 ui/x_keymap.h
>>>>>
>>>>> diff --git a/Makefile b/Makefile
>>>>> index 2bbc547..f776c30 100644
>>>>> --- a/Makefile
>>>>> +++ b/Makefile
>>>>> @@ -116,7 +116,45 @@ QEMU_CFLAGS+=$(GLIB_CFLAGS)
>>>>>
>>>>>   ui/cocoa.o: ui/cocoa.m
>>>>>
>>>>> -ui/sdl.o audio/sdlaudio.o ui/sdl_zoom.o baum.o: QEMU_CFLAGS += $(SDL_CFLAGS)
>>>>> +ui/sdl.o audio/sdlaudio.o ui/sdl_zoom.o ui/sdl_keymap.o baum.o: QEMU_CFLAGS += $(SDL_CFLAGS)
>>>>> +
>>>>> +KEYMAP_GEN = ui/keymap-gen.pl
>>>>> +KEYMAP_CSV = ui/keymaps.csv
>>>>> +
>>>>> +SDL_KEYMAPS = \
>>>>> +        ui/sdl_keymap_xorgevdev2rfb.c \
>>>>> +        ui/sdl_keymap_xorgkbd2rfb.c \
>>>>> +        ui/sdl_keymap_xorgxquartz2rfb.c \
>>>>> +        ui/sdl_keymap_xorgxwin2rfb.c \
>>>>> +        ui/sdl_keymap_osx2rfb.c \
>>>>> +        ui/sdl_keymap_win322rfb.c
>>>>> +
>>>>> +$(SDL_KEYMAPS): $(KEYMAP_GEN) $(KEYMAP_CSV)
>>>>> +GENERATED_SOURCES += $(SDL_KEYMAPS)
>>>>> +
>>>>> +# Avoid need for perl(Text::CSV) by end users
>>>>> +# XXXX how does QEMU make file deal with this
>>>>> +#EXTRA_DIST += $(SDL_KEYMAPS)
>>>>> +
>>>>> +ui/sdl_keymap.c: $(SDL_KEYMAPS)
>>>>> +
>>>>> +ui/sdl_keymap_xorgevdev2rfb.c: $(KEYMAP_CSV)
>>>>> +	$(call quiet-command,perl $(KEYMAP_GEN) $<    xorgevdev rfb>    $@ || rm $@, "  GEN $@")
>>>>
>>>> I'm not sure I'm prepared to add a perl build dependency.  That's
>>>> particularly hard for Windows users.  We could alternatively use a
>>>> python version of keymap-gen but then we're forking from
>>>> gtk-vnc/gtk-spice.
>>>
>>> I expected as much, which is why I sent the 2nd patch which actually
>>> adds the generated files to GIT, so Perl is not required unless
>>> someone decides to update the keymaps.csv file.
>>>
>>>> Maybe it makes sense to create a small library for dealing with
>>>> keymaps?  Then we could all link against it and probe for that
>>>> conditionally?
>>>
>>> I don't think it is worth doing a library just for the keymap
>>> tables. It could be worth creating a set of add-on libraries
>>> (eg libsdl-keymap, libgdk-keymap, etc) for doing the keymap
>>> hueristics.  In the immediate future though, my intent is to
>>> request a GIT repository on freedesktop.org to host the keymaps.csv
>>> file and a generator script, which could just be just included in
>>> apps using a GIT submodule approach.
>>
>> Ah, we already do git submodules so I'd much prefer we wait to do
>> this.  Is this something that you are going to do in the very near
>> future or only sort of near future?
>
> I can probably get it done in the next week or two

Okay, let me know, and I can help with the submodule bits.

Thanks,

Anthony Liguori

>
> Daniel