[libvirt] [PATCH 1/3 V3] lib: add virtkey

[Lai Jiangshan]

On 06/24/2011 09:18 PM, Daniel P. Berrange wrote:
> On Fri, Jun 24, 2011 at 02:33:29PM +0800, Lai Jiangshan wrote:
>> Add virtkey lib for usage-improvment and keycode translating.
>> Add 4 internal API for the aim
>>
>> const char *virKeycodeSetName(virKeycodeSet codeset);
>> virKeycodeSet virParseKeycodeSet(const char *name);
> 
> These should just be done using the standard VIR_ENUM_DECL/IMPL macros.
> 
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index 3f634e6..2f2efe7 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -1815,6 +1815,12 @@ typedef enum {
>>      VIR_KEYCODE_SET_ATSET1         = 2,
>>      VIR_KEYCODE_SET_ATSET2         = 3,
>>      VIR_KEYCODE_SET_ATSET3         = 4,
>> +    VIR_KEYCODE_SET_OSX            = 5,
>> +    VIR_KEYCODE_SET_XT_KBD         = 6,
>> +    VIR_KEYCODE_SET_USB            = 7,
>> +    VIR_KEYCODE_SET_WIN32          = 8,
>> +    VIR_KEYCODE_SET_XWIN_XT        = 9,
>> +    VIR_KEYCODE_SET_XFREE86_KBD_XT = 10,
>>  } virKeycodeSet;
> 
> IMHO, we don't really need to include the XT_KBD, XWIN_XT or
> XFREE86_KBD_XT codesets, since these are all special purpose
> sets which are just derived from the based XT set. Lets just
> stick to the core interesting sets. So add OSX, USB and WIN32
> only.

I found qemu monitor just accept XT_KBD, not XT, maybe I'm wrong.

> 
>> diff --git a/src/util/virtkey.c b/src/util/virtkey.c
>> new file mode 100644
>> index 0000000..48fbfcc
>> --- /dev/null
>> +++ b/src/util/virtkey.c
>> @@ -0,0 +1,633 @@
>> +
>> +/*
>> + * Copyright (c) 2011 Lai Jiangshan
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include <config.h>
>> +#include <string.h>
>> +#include <stddef.h>
>> +#include <libvirt/libvirt.h>
>> +#include "virtkey.h"
>> +
>> +#define ARRAY_SIZE(array) (sizeof(array)/sizeof(array[0]))
>> +#define getfield(object, field_type, field_offset) \
>> +    (*(typeof(field_type) *)((char *)(object) + field_offset))
>> +
>> +struct keycode {
>> +    const char *linux_name;
>> +    const char *os_x_name;
>> +    const char *win32_name;
>> +    unsigned short linux_keycode;
>> +    unsigned short xt;
>> +    unsigned short atset1;
>> +    unsigned short atset2;
>> +    unsigned short atset3;
>> +    unsigned short os_x;
>> +    unsigned short xt_kbd;
>> +    unsigned short usb;
>> +    unsigned short win32;
>> +    unsigned short xwin_xt;
>> +    unsigned short xfree86_kbd_xt;
>> +};
>> +
>> +/*
>> + * generated from http://git.gnome.org/browse/gtk-vnc/plain/src/keymaps.csv
>> + * script:
>> + *
>> + * #!/bin/python
>> + * import sys
>> + * import re
>> + *
>> + * for line in sys.stdin.xreadlines():
>> + *     a = re.match("([^,]*)," * 13 + "([^,]*)$", line[0:-1]).groups()
>> + *     b = ""
>> + *     for i in (0,2,10,1,7,4,5,6,3,8,9,11,12,13):
>> + *         if i in (0, 2, 10):
>> + *             b = b + (a[i] and ('"' + a[i] + '"') or 'NULL') + ','
>> + *         else:
>> + *             b = b + (a[i] or '0') + ','
>> + *     print "    { " + b + "},"
>> + */
> 
> One of the goals of having the keymap data in the CSV file was that
> it makes it trivially updatable across apps using it, without making
> code changes. In fact my goal is to actually put 'keymaps.csv' and
> 'keymaps.pl' into a separate shared package at some point. So rather
> than hardcoding this giant array in libvirt, just include the GTK-VNC
> keymaps.csv and keymaps.pl file as-is, and run them to generate the
> mapping tables for combinations we need.
> 
> NB, keymaps.pl will need to be updated to be able to output a
> table for doing "string->keycode" mapping since it doesn't
> do that yet. For the plain  keycode->keycode mappings though
> just use its currently functionality.

I didn't find separate git repository for  keymaps.csv.
Should I copy keymaps.csv to libvirt?

keymaps.pl need to be run O(N*N) times and it will generate O(N*N) tables
for different translating, I think that 1 table is the best, even the
table are bigger.


> 
>> +const char *virKeycodeSetName(virKeycodeSet codeset)
>> +{
>> +    int i = (int)codeset;
>> +
>> +    if (i < 0 || i >= ARRAY_SIZE(codesetInfo))
>> +        return "UNKNOWN";
>> +
>> +    return codesetInfo[i].name;
>> +}
>> +
>> +virKeycodeSet virParseKeycodeSet(const char *name)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(codesetInfo); i++) {
>> +        if (!strcmp(codesetInfo[i].name, name))
>> +            return (virKeycodeSet)i;
>> +    }
>> +
>> +    return (virKeycodeSet)-1;
>> +}
> 
> These just get replaced by VIR_ENUM_IMPL

Will do, thanks,

> 
>> +static int virParseKeyNameOffset(unsigned int name_offset,
>> +                                 unsigned int code_offset,
>> +                                 const char *keyname)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(keycodes); i++) {
>> +        const char *name = getfield(keycodes + i, const char *, name_offset);
>> +
>> +        if (name && !strcmp(name, keyname))
>> +            return getfield(keycodes + i, unsigned short, code_offset);
>> +    }
>> +
>> +    return -1;
>> +}
> 
> This will want to use a number table that keymaps.pl will
> need to generate for name -> code mapping.
> 
>> +static int virTranslateKeyCodeOffset(unsigned int from_offset,
>> +                                     unsigned int to_offset,
>> +                                     int key_value)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(keycodes); i++) {
>> +        if (getfield(keycodes + i, unsigned short, from_offset) == key_value)
>> +            return getfield(keycodes + i, unsigned short, to_offset);
>> +    }
>> +
>> +    return -1;
>> +}
> 
> This is not nice because it is O(n) lookups. If you just use
> the keymaps.pl script to generate all the conversion tables
> we need, we get O(1) lookups & simpler code.

I think O(n) lookups is OK for <=16 keycodes.

Thank you very much. I need to investigate/think more
Lai

> 
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index fcd254d..a1e2f83 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -58,6 +58,7 @@
>>  #include "threads.h"
>>  #include "command.h"
>>  #include "count-one-bits.h"
>> +#include "virtkey.h"
>>  
>>  static char *progname;
> 
> This ought to be in the next patch, since this doesn't need it yet
> 
> Regards,
> Daniel