From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LL1U1-0006gl-3D for qemu-devel@nongnu.org; Thu, 08 Jan 2009 15:28:01 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LL1Tz-0006fi-Fq for qemu-devel@nongnu.org; Thu, 08 Jan 2009 15:27:59 -0500 Received: from [199.232.76.173] (port=34092 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LL1Tz-0006fa-Bm for qemu-devel@nongnu.org; Thu, 08 Jan 2009 15:27:59 -0500 Received: from qw-out-1920.google.com ([74.125.92.147]:47161) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LL1Ty-0004N9-RH for qemu-devel@nongnu.org; Thu, 08 Jan 2009 15:27:59 -0500 Received: by qw-out-1920.google.com with SMTP id 5so4184747qwc.4 for ; Thu, 08 Jan 2009 12:27:58 -0800 (PST) Message-ID: <496661C6.3010507@codemonkey.ws> Date: Thu, 08 Jan 2009 14:27:50 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1 of 3] Fix keymap handling for vnc console References: <4947CA2B.5030401@oracle.com> <4947CD50.8040806@oracle.com> In-Reply-To: <4947CD50.8040806@oracle.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org John Haxby wrote: > Fix keymap handling for international keyboards > > Signed-off-by: John Haxby > > 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