From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SrToT-0003kM-KF for qemu-devel@nongnu.org; Wed, 18 Jul 2012 08:57:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SrToJ-0007Mh-7V for qemu-devel@nongnu.org; Wed, 18 Jul 2012 08:57:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36682) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SrToI-0007Mb-Sc for qemu-devel@nongnu.org; Wed, 18 Jul 2012 08:56:59 -0400 Message-ID: <5006B296.2000409@redhat.com> Date: Wed, 18 Jul 2012 20:56:54 +0800 From: Amos Kong MIME-Version: 1.0 References: <1341492525-29809-1-git-send-email-akong@redhat.com> <1341492525-29809-6-git-send-email-akong@redhat.com> <20120712120907.0d22c66b@doriath.home> In-Reply-To: <20120712120907.0d22c66b@doriath.home> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 5/6] qapi: convert sendkey List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@us.ibm.com, eblake@redhat.com, qemu-devel@nongnu.org On 12/07/12 23:09, Luiz Capitulino wrote: > Hi Luiz, > On Thu, 5 Jul 2012 20:48:44 +0800 > Amos Kong wrote: > >> Convert 'sendkey' to use QAPI. do_sendkey() depends on some >> variables/functions in monitor.c, so reserve qmp_sendkey() >> to monitor.c >> >> key_defs[] in console.h is the mapping of key name to keycode, >> Keys' index in the enmu and key_defs[] is same. >> >> 'send-key' of QMP doesn't support key in hexadecimal format. >> >> Signed-off-by: Amos Kong >> --- >> console.h | 152 ++++++++++++++++++++++++++++++++++ >> hmp-commands.hx | 2 +- >> hmp.c | 64 +++++++++++++++ >> hmp.h | 1 + >> monitor.c | 239 ++++++------------------------------------------------ >> qapi-schema.json | 46 +++++++++++ >> qmp-commands.hx | 28 +++++++ >> 7 files changed, 317 insertions(+), 215 deletions(-) >> >> diff --git a/console.h b/console.h >> index 4334db5..e1b0c45 100644 >> --- a/console.h >> +++ b/console.h >> @@ -6,6 +6,7 @@ >> #include "notify.h" >> #include "monitor.h" >> #include "trace.h" >> +#include "qapi-types.h" >> >> /* keyboard/mouse support */ >> >> @@ -397,4 +398,155 @@ static inline int vnc_display_pw_expire(DisplayState *ds, time_t expires) >> /* curses.c */ >> void curses_display_init(DisplayState *ds, int full_screen); >> >> +typedef struct { >> + int keycode; >> + const char *name; > > I don't think we need 'name', as key names are also provided by KeyCodes_lookup[]. > See more on this below. Yes, I tried to define key_defs[] to a int array, and get keyname from KeyCodes_loopup[], it works. const int key_defs[] = { [KEY_CODES_SHIFT] = 0x2a, .... >> +} 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. { '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(). then convert this 'index' to keycode in qmp_send_key() I didn't find a way to define a non-serial enum. 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. >> + 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. Will fix other issues you mentioned, thanks for your review. -- Amos.