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

[Anthony Liguori <anthony@codemonkey.ws>]

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'm not wedded to Perl for the generator, so if it is more
> convenient to use another language, even C, I can easily do
> that too.
>
> Is it acceptable to just commit the generated tables to QEMU GIT
> in the immediate term though, so users can get the fixes without
> needing perl ?

I'd rather go the git submodule approach than copy code from a separate project. 
  I can include the .c files in the tree based on the submodule and we can 
update the .c files whenever the submodule is bumped.

Regards,

Anthony Liguori

> Daniel