----- Original Message ----- > On Wed, 18 Jul 2012 20:56:54 +0800 > Amos Kong <ak...@redhat.com> wrote: > > > >> +} KeyDef; > > >> + > > >> +static const KeyDef key_defs[] = { > > > > > > We can't have an array defined in a header file because it will > > > be defined in > > > each .c file that includes it. > > > > > > Please, define it in input.c (along with qmp_send_key()) > > > > Ok. > > > > > and write the following public functions: > > > > > > o KeyCode keycode_from_key(const char *key); > > > o KeyCode keycode_from_code(int code); > > > > > > void qmp_send_key(KeyCodesList *keys, bool has_hold_time, int64_t > > hold_time, ...) > > ^ > > \_ when we use qmp, a key list will be passed, > > the > > values are the index > > in enum KeyCodes. not the real KeyCode. > > Right. > > > > > { 'enum': 'KeyCodes', > > 'data': [ 'shift', 'shift_r', 'al... > > > > So we need to get this kind of 'index' in hmp_send_key() and pass > > to > > qmp_send_key(). > > Yes, that's what keycode_from_key() would do, something like this: > > KeyCode keycode_from_key(const char *key) > { > int i; > > for (i = 0; i < KEY_CODES_MAX; i++) { > if (!strcmp(key, KeyCode_lookup[i])) { > return i; > } > } > > return KEY_CODE_MAX; > } > > Note that it returns the KeyCode index, and should be defined in > input.c.
Sure :) > > then convert this 'index' to keycode in qmp_send_key() > > Exactly, qmp_send_key() can access key_defs[] to get the keycode from > the > index. > > > > > I didn't find a way to define a non-serial enum. > > I'm not sure I follow you here, I think that what I suggested above > will work. So I would continually pass 'index' to qmp_send_key(). I already implemented those in localhost, they all works. Will fix other issues and post v5 later, thanks. > > eg: (then int qmp_marshal_input_send_key() would pass real keycode > > to > > qmp_send_key()) > > { 'enum': 'KeyCodes', > > 'data': [ 'shift' = 0x2a, 'shift_r' = 0x36, 'alt' = 0x38, ... > > > > > > If we still pass 'index' to qmp_send_key as patch V4. > > > > extern int index_from_key(const char *key); -> it's used in > > hmp_send_key() > > extern int index_from_keycode(int code); -> it's used in > > hmp_send_key() > > extern char *key_from_keycode(int idx); -> it's used in > > monitor_find_completion() > > extern int keycode_from_key(const char *key); -> it's used in > > qmp_send_key() > > > > > > > and then use these functions where using key_defs would be > > > necessary. Also, > > > note that keycode_from_key() can use KeyCodes_lookup[] instead of > > > key_defs (this > > > way we can drop 'name' from KeyDef). > > > > .... > > > > >> +#endif > > >> +#endif > > >> + [KEY_CODES_MAX] = { 0, NULL }, > > >> +}; > > >> + > > >> #endif > > >> diff --git a/hmp-commands.hx b/hmp-commands.hx > > >> index e336251..865eea9 100644 > > >> --- a/hmp-commands.hx > > >> +++ b/hmp-commands.hx > > >> @@ -505,7 +505,7 @@ ETEXI > > >> .args_type = "keys:s,hold-time:i?", > > >> .params = "keys [hold_ms]", > > >> .help = "send keys to the VM (e.g. 'sendkey > > >> ctrl-alt-f1', default hold time=100 ms)", > > >> - .mhandler.cmd = do_sendkey, > > >> + .mhandler.cmd = hmp_send_key, > > >> }, > > >> > > >> STEXI > > >> diff --git a/hmp.c b/hmp.c > > >> index b9cec1d..cfdc106 100644 > > >> --- a/hmp.c > > >> +++ b/hmp.c > > >> @@ -19,6 +19,7 @@ > > >> #include "qemu-timer.h" > > >> #include "qmp-commands.h" > > >> #include "monitor.h" > > >> +#include "console.h" > > >> > > >> static void hmp_handle_error(Monitor *mon, Error **errp) > > >> { > > >> @@ -1000,3 +1001,66 @@ void hmp_netdev_del(Monitor *mon, const > > >> QDict *qdict) > > >> qmp_netdev_del(id,&err); > > >> hmp_handle_error(mon,&err); > > >> } > > >> + > > >> +static int get_key_index(const char *key) > > >> +{ > > >> + int i, keycode; > > >> + char *endp; > > >> + > > >> + for (i = 0; i< KEY_CODES_MAX; i++) > > >> + if (key_defs[i].keycode&& !strcmp(key, > > >> key_defs[i].name)) > > >> + return i; > > > > > > Here you can call do: > > > > > > keycode = keycode_from_key(key); > > > if (keycode != KEY_CODES_MAX) { > > > return keycode; > > > } > > > > > >> + > > >> + if (strstart(key, "0x", NULL)) { > > >> + keycode = strtoul(key,&endp, 0); > > >> + if (*endp == '\0'&& keycode>= 0x01&& keycode<= 0xff) > > >> + for (i = 0; i< KEY_CODES_MAX; i++) > > >> + if (keycode == key_defs[i].keycode) > > >> + return i; > > > > > > You can drop that for loop and do instead: > > > > > > keycode = keycode_from_code(keycode); > > > > > > > > >> + } > > >> + > > >> + return -1; > > >> +} > > >> + > > >> +void hmp_send_key(Monitor *mon, const QDict *qdict) > > >> +{ > > >> + const char *keys = qdict_get_str(qdict, "keys"); > > >> + KeyCodesList *keylist, *head = NULL, *tmp = NULL; > > >> + int has_hold_time = qdict_haskey(qdict, "hold-time"); > > >> + int hold_time = qdict_get_try_int(qdict, "hold-time", -1); > > >> + Error *err = NULL; > > >> + char keyname_buf[16]; > > >> + char *separator; > > >> + int keyname_len; > > >> + > > >> + while (1) { > > >> + separator = strchr(keys, '-'); > > >> + keyname_len = separator ? separator - keys : > > >> strlen(keys); > > >> + pstrcpy(keyname_buf, sizeof(keyname_buf), keys); > > >> + > > >> + /* Be compatible with old interface, convert user > > >> inputted "<" */ > > >> + if (!strncmp(keyname_buf, "<", 1)&& keyname_len == 1) > > >> { > > >> + pstrcpy(keyname_buf, sizeof(keyname_buf), "less"); > > >> + keyname_len = 4; > > >> + } > > >> + keyname_buf[keyname_len] = 0; > > >> + > > >> + keylist = g_malloc0(sizeof(*keylist)); > > >> + keylist->value = get_key_index(keyname_buf); > > > > > > get_key_index() can fail. > > > > Ok, I would check it. > > > > >> + keylist->next = NULL; > > >> + > > >> + if (tmp) > > >> + tmp->next = keylist; > > >> + tmp = keylist; > > >> + if (!head) > > >> + head = keylist; > > >> + > > >> + if (!separator) > > >> + break; > > >> + keys = separator + 1; > > >> + } > > > > ... > > > > >> - { 0xfe, "compose" }, > > >> -#endif > > >> - { 0, NULL }, > > >> -}; > > >> - > > >> -static int get_keycode(const char *key) > > >> -{ > > >> - const KeyDef *p; > > >> - char *endp; > > >> - int ret; > > >> > > >> - for(p = key_defs; p->name != NULL; p++) { > > >> - if (!strcmp(key, p->name)) > > >> - return p->keycode; > > >> - } > > >> - if (strstart(key, "0x", NULL)) { > > >> - ret = strtoul(key,&endp, 0); > > >> - if (*endp == '\0'&& ret>= 0x01&& ret<= 0xff) > > >> - return ret; > > >> - } > > >> - return -1; > > >> -} > > >> - > > >> -#define MAX_KEYCODES 16 > > >> -static uint8_t keycodes[MAX_KEYCODES]; > > >> -static int nb_pending_keycodes; > > >> +static KeyCodesList *keycodes; > > >> static QEMUTimer *key_timer; > > >> > > >> static void release_keys(void *opaque) > > >> { > > >> int keycode; > > >> + KeyCodesList *p; > > >> + > > >> + for (p = keycodes; p != NULL; p = p->next) { > > >> + if (p->value> KEY_CODES_MAX) { > > > > > > This check is not needed, as far as I can understand only > > > qmp_send_key() can put > > > keys in the keycodes list and qmp_send_key() does this check > > > already. > > > > If we find one invalid 'value', other keys in the list will be > > ignored. > > so we don't need to release them here. > > That should be done in qmp_send_key(), only valid keys should be > passed to > release_keys(). Ok. will use two loops. > > > > > > >> + keycodes = NULL; > > >> + break; > > >> + } > > >> > > >> - while (nb_pending_keycodes> 0) { > > >> - nb_pending_keycodes--; > > >> - keycode = keycodes[nb_pending_keycodes]; > > >> + keycode = key_defs[p->value].keycode; > > >> if (keycode& 0x80) > > >> kbd_put_keycode(0xe0); > > >> kbd_put_keycode(keycode | 0x80); > > >> } > > > > > > Please set keycodes to NULL here. Actually, you'll have to free > > > it first, > > > because keycodes has to be duplicated (see below). > > > > > >> } > > >> > > >> -static void do_sendkey(Monitor *mon, const QDict *qdict) > > >> +void qmp_send_key(KeyCodesList *keys, bool has_hold_time, > > >> int64_t hold_time, > > >> + Error **errp) > > >> { > > >> - char keyname_buf[16]; > > >> - char *separator; > > >> - int keyname_len, keycode, i; > > >> - const char *keys = qdict_get_str(qdict, "keys"); > > >> - int has_hold_time = qdict_haskey(qdict, "hold-time"); > > >> - int hold_time = qdict_get_try_int(qdict, "hold-time", -1); > > >> + int keycode; > > >> + char value[5]; > > >> + KeyCodesList *p; > > > > > > Let's initialize key_timer here, like this: > > > > > > if (!key_timer) { > > > key_timer = qemu_new_timer_ns(vm_clock, release_keys, NULL); > > > } > > > > > > Then drop the initialization done in monitor_init(). This way we > > > untangle > > > qmp_send_key() from the monitor. > > > > > > Also, please, move qmp_send_key(), release_keys, etc to input.c > > > as I said > > > above. > > > > Ok. > > > > >> - if (nb_pending_keycodes> 0) { > > >> + if (keycodes != NULL) { > > >> qemu_del_timer(key_timer); > > >> release_keys(NULL); > > >> } > > >> if (!has_hold_time) > > >> hold_time = 100; > > > > > > Please, add { } around the if body above. > > > > > >> - i = 0; > > >> - while (1) { > > >> - separator = strchr(keys, '-'); > > >> - keyname_len = separator ? separator - keys : > > >> strlen(keys); > > >> - if (keyname_len> 0) { > > >> - pstrcpy(keyname_buf, sizeof(keyname_buf), keys); > > >> - if (keyname_len> sizeof(keyname_buf) - 1) { > > >> - monitor_printf(mon, "invalid key: '%s...'\n", > > >> keyname_buf); > > >> - return; > > >> - } > > >> - if (i == MAX_KEYCODES) { > > >> - monitor_printf(mon, "too many keys\n"); > > >> - return; > > >> - } > > >> > > >> - /* Be compatible with old interface, convert user > > >> inputted "<" */ > > >> - if (!strncmp(keyname_buf, "<", 1)&& keyname_len == > > >> 1) { > > >> - pstrcpy(keyname_buf, sizeof(keyname_buf), > > >> "less"); > > >> - keyname_len = 4; > > >> - } > > >> + keycodes = keys; > > > > > > It's better to this assignment after the for loop below. > > > Actually, you have to > > > duplicate the key list because the qapi code will free it as soon > > > as > > > qmp_send_key() returns, but it will be used later in the timer > > > handler. > > > > > >> > > >> - keyname_buf[keyname_len] = 0; > > >> - keycode = get_keycode(keyname_buf); > > >> - if (keycode< 0) { > > >> - monitor_printf(mon, "unknown key: '%s'\n", > > >> keyname_buf); > > >> - return; > > >> - } > > >> - keycodes[i++] = keycode; > > >> + for (p = keycodes; p != NULL; p = p->next) { > > >> + if (p->value> KEY_CODES_MAX) { > > > > > > You should test against>=. > > > > > >> + sprintf(value, "%d", p->value); > > >> + error_set(errp, QERR_INVALID_PARAMETER, value); > > > > ^^ If an invalid key is found, the other keys will be ignored. > > Well, that's the behavior I'd expect. Maybe we should loop twice. > First > we check everything is ok and then we trigger the key down events. > > Now, the current code doesn't do this and I'm not sure if we would > break compatibility... I'll let you choose what you think is best. > > > > > > > Will fix other issues you mentioned, thanks for your review. > > > > >