Re: [Qemu-devel] [PATCH 1 of 3] Fix keymap handling for vnc console

[Anthony Liguori <anthony@codemonkey.ws>]

John Haxby wrote:
> Fix keymap handling for international keyboards
>
> Signed-off-by: John Haxby <john.haxby@oracle.com>
>
> curses_keys.h |    2
> keymaps.c     |  347 
> ++++++++++++++++++++++++++++++++++++----------------------
> sdl_keysym.h  |    2
> vnc.c         |   87 ++++++++++----
> vnc_keysym.h  |    2
> 5 files changed, 286 insertions(+), 154 deletions(-)
>
> diff -up qemu-0.9.1.6042/curses_keys.h.keymap01 
> qemu-0.9.1.6042/curses_keys.h
> --- qemu-0.9.1.6042/curses_keys.h.keymap01    2008-10-28 
> 00:11:06.000000000 +0000
> +++ qemu-0.9.1.6042/curses_keys.h    2008-12-16 10:47:36.000000000 +0000
> @@ -244,7 +244,7 @@ typedef struct {
>     int keysym;
> } name2keysym_t;
>
> -static const name2keysym_t name2keysym[] = {
> +static name2keysym_t name2keysym[] = {
>     /* Plain ASCII */
>     { "space", 0x020 },
>     { "exclam", 0x021 },
> diff -up qemu-0.9.1.6042/keymaps.c.keymap01 qemu-0.9.1.6042/keymaps.c
> --- qemu-0.9.1.6042/keymaps.c.keymap01    2008-10-02 
> 19:26:42.000000000 +0100
> +++ qemu-0.9.1.6042/keymaps.c    2008-12-16 10:47:36.000000000 +0000
> @@ -22,58 +22,64 @@
>  * THE SOFTWARE.
>  */
>
> +static int cmp_keysym(const void *a, const void *b) {
> +    return strcmp(((name2keysym_t *) a)->name, ((name2keysym_t *) 
> b)->name);
> +}
> +
> +
> static int get_keysym(const char *name)
> {
> -    const name2keysym_t *p;
> -    for(p = name2keysym; p->name != NULL; p++) {
> -        if (!strcmp(p->name, name))
> -            return p->keysym;
> +    static int n = -1;
> +    int l, r, m;
> +
> +    if (n < 0) {
> +    for (n = 0; name2keysym[n].name; n++);
> +    qsort(name2keysym, n, sizeof(name2keysym_t), cmp_keysym);

Doing an in place sort of a literal array is probably not a great 
idea.   Why does the array need sorting?  The indenting is really 
screwed up here.

> +    }
> +    l = 0;
> +    r = n-1;
> +    while (l <= r) {
> +    m = (l + r) / 2;
> +    int cmp = strcmp(name2keysym[m].name, name);

Please don't mix variable declarations with code.

> +    if (cmp < 0)
> +        l = m + 1;
> +    else if (cmp > 0)
> +        r = m - 1;
> +    else
> +        return name2keysym[m].keysym;
> +    }

This is an open coded binary search?  Why not just do a linear search?  
Is this really a performance concern?

> +    if (name[0] == 'U') {
> +    /* unicode symbol key */
> +    char *ptr;
> +    int k = strtol(name+1, &ptr, 16);
> +    return *ptr ? 0 : k;
>     }
> -    return 0;
> }
>
> -struct key_range {
> -    int start;
> -    int end;
> -    struct key_range *next;
> -};
> +#define MAX_SCANCODE 256
> +#define KEY_LOCALSTATE 0x1
> +#define KEY_KEYPAD 0x2
>
> #define MAX_NORMAL_KEYCODE 512
> #define MAX_EXTRA_COUNT 256
> typedef struct {
> -    uint16_t keysym2keycode[MAX_NORMAL_KEYCODE];
> -    struct {
> -    int keysym;
> -    uint16_t keycode;
> -    } keysym2keycode_extra[MAX_EXTRA_COUNT];
> -    int extra_count;
> -    struct key_range *keypad_range;
> -    struct key_range *numlock_range;
> -} kbd_layout_t;
> +    int keysym;
> +    int keycode;
> +} keysym2keycode_t;
>
> -static void add_to_key_range(struct key_range **krp, int code) {
> -    struct key_range *kr;
> -    for (kr = *krp; kr; kr = kr->next) {
> -    if (code >= kr->start && code <= kr->end)
> -        break;
> -    if (code == kr->start - 1) {
> -        kr->start--;
> -        break;
> -    }
> -    if (code == kr->end + 1) {
> -        kr->end++;
> -        break;
> -    }
> -    }
> -    if (kr == NULL) {
> -    kr = qemu_mallocz(sizeof(*kr));
> -    if (kr) {
> -        kr->start = kr->end = code;
> -        kr->next = *krp;
> -        *krp = kr;
> -    }
> -    }
> -}
> +typedef struct {
> +    int n;
> +    keysym2keycode_t k[MAX_SCANCODE];
> +} keysym_map_t;
> +
> +typedef struct {
> +    keysym_map_t plain;
> +    keysym_map_t shift;
> +    keysym_map_t altgr;
> +    keysym_map_t shift_altgr;
> +    keysym_map_t numlock;
> +    uint32_t flags [MAX_SCANCODE];
> +} kbd_layout_t;
>
> static kbd_layout_t *parse_keyboard_layout(const char *language,
>                        kbd_layout_t * k)
> @@ -81,105 +87,194 @@ static kbd_layout_t *parse_keyboard_layo
>     FILE *f;
>     char file_name[1024];
>     char line[1024];
> -    int len;
>
>     snprintf(file_name, sizeof(file_name),
> -             "%s/keymaps/%s", bios_dir, language);
> +         "%s/keymaps/%s", bios_dir, language);

Please don't change existing code formatting.

>     if (!k)
>     k = qemu_mallocz(sizeof(kbd_layout_t));
>     if (!k)
> -        return 0;
> +    return 0;

Like right here.

>     if (!(f = fopen(file_name, "r"))) {
>     fprintf(stderr,
>         "Could not read keymap file: '%s'\n", file_name);
>     return 0;
>     }
> -    for(;;) {
> -    if (fgets(line, 1024, f) == NULL)
> -            break;
> -        len = strlen(line);
> -        if (len > 0 && line[len - 1] == '\n')
> -            line[len - 1] = '\0';
> -        if (line[0] == '#')
> -        continue;
> -    if (!strncmp(line, "map ", 4))
> -        continue;
> -    if (!strncmp(line, "include ", 8)) {
> -        parse_keyboard_layout(line + 8, k);
> -        } else {
> -        char *end_of_keysym = line;
> -        while (*end_of_keysym != 0 && *end_of_keysym != ' ')
> -        end_of_keysym++;
> -        if (*end_of_keysym) {
> -        int keysym;
> -        *end_of_keysym = 0;
> -        keysym = get_keysym(line);
> -        if (keysym == 0) {
> -                    //            fprintf(stderr, "Warning: unknown 
> keysym %s\n", line);
> -        } else {
> -            const char *rest = end_of_keysym + 1;
> -            char *rest2;
> -            int keycode = strtol(rest, &rest2, 0);
> -
> -            if (rest && strstr(rest, "numlock")) {
> -            add_to_key_range(&k->keypad_range, keycode);
> -            add_to_key_range(&k->numlock_range, keysym);
> -            //fprintf(stderr, "keypad keysym %04x keycode %d\n", 
> keysym, keycode);
> -            }
> -
> -            /* if(keycode&0x80)
> -               keycode=(keycode<<8)^0x80e0; */
> -            if (keysym < MAX_NORMAL_KEYCODE) {
> -            //fprintf(stderr,"Setting keysym %s (%d) to 
> %d\n",line,keysym,keycode);
> -            k->keysym2keycode[keysym] = keycode;
> -            } else {
> -            if (k->extra_count >= MAX_EXTRA_COUNT) {
> -                fprintf(stderr,
> -                    "Warning: Could not assign keysym %s (0x%x) 
> because of memory constraints.\n",
> -                    line, keysym);
> -            } else {
> -#if 0
> -                fprintf(stderr, "Setting %d: %d,%d\n",
> -                    k->extra_count, keysym, keycode);
> -#endif
> -                k->keysym2keycode_extra[k->extra_count].
> -                keysym = keysym;
> -                k->keysym2keycode_extra[k->extra_count].
> -                keycode = keycode;
> -                k->extra_count++;
> -            }
> -            }
> -        }
> -        }
> +    while (fgets(line, 1024, f)) {
> +    char *ptr = strchr(line, '#');
> +    char keyname[1024], p1[1024], p2[1024];
> +    int keysym, keycode;
> +    int shift = 0;
> +    int altgr = 0;
> +    int addupper = 0;
> +    int numlock = 0;
> +    int inhibit = 0;

Again, the formatting is off.

> +    if (ptr)
> +        *ptr-- = '\0';

What happens if the comment character is the first character in the 
line  You'll under run the buffer.

> +    else
> +        ptr = &line[strlen(line)-1];
> +    while (isspace(*ptr))
> +        *ptr-- = '\0';
> +    if (!*line)
> +        continue;
> +    if (strncmp(line, "map ", 4) == 0)
> +        continue;
> +    if (sscanf(line, "include %s", p1) == 1) {

%s is generally unsafe to use with sscanf unless you use .* too.

Regards,

Anthony Liguori