| Message ID | 1340167661-25499-6-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/19/2012 10:47 PM, 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 monitor.c is the mapping of key name to keycode, > Keys' order in the enmu and key_defs[] is same. > The interface looks like we have settled into something useful that libvirt can live with. I will leave a more thorough review of the series to those more familiar with qemu's internals. However, as long as we are fine-tuning things, I have some questions that might be worth one more spin: > +## > +# @sendkey: Should this be named 'send-key', given that most QMP commands separate words with '-' rather than squasheverythingtogether? > +SQMP > +sendkey > +---------- > + > +Send keys to VM. > + > +Arguments: > + > +keys array: > + - "key": key sequence (json-string) I'm not sure if json-string is the right designation any more. Rather, it is a json-array of key enum values. > + > +- hold-time: time to delay key up events, milliseconds (josn-int, optional) s/josn/json/
On Wed, 20 Jun 2012 12:47:40 +0800 Amos Kong <akong@redhat.com> 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 monitor.c is the mapping of key name to keycode, > Keys' order in the enmu and key_defs[] is same. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > hmp-commands.hx | 2 +- > hmp.c | 45 ++++++++++++++++++++++++ > hmp.h | 1 + > monitor.c | 102 +++++++++++++++++++++++------------------------------ > monitor.h | 2 + > qapi-schema.json | 45 ++++++++++++++++++++++++ > qmp-commands.hx | 27 ++++++++++++++ > 7 files changed, 165 insertions(+), 59 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index e336251..fee4c14 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_sendkey, > }, > > STEXI > diff --git a/hmp.c b/hmp.c > index b9cec1d..846e8b9 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -19,6 +19,7 @@ > #include "qemu-timer.h" > #include "qmp-commands.h" > #include "monitor.h" > +#include "qapi-types.h" > > static void hmp_handle_error(Monitor *mon, Error **errp) > { > @@ -1000,3 +1001,47 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) > qmp_netdev_del(id, &err); > hmp_handle_error(mon, &err); > } > + > +void hmp_sendkey(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]; > + Please, drop this blank line. > + 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); > + keylist->next = NULL; > + > + if (tmp) > + tmp->next = keylist; > + tmp = keylist; > + if (!head) > + head = keylist; > + > + if (!separator) > + break; > + keys = separator + 1; > + } > + > + qmp_sendkey(head, has_hold_time, hold_time, &err); > + hmp_handle_error(mon, &err); > + qapi_free_KeyCodesList(head); > +} > diff --git a/hmp.h b/hmp.h > index 79d138d..bcc74d2 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); > void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); > void hmp_netdev_add(Monitor *mon, const QDict *qdict); > void hmp_netdev_del(Monitor *mon, const QDict *qdict); > +void hmp_sendkey(Monitor *mon, const QDict *qdict); > > #endif > diff --git a/monitor.c b/monitor.c > index 6fa0104..863c0c6 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -1470,98 +1470,84 @@ static const KeyDef key_defs[] = { > { 0, NULL }, > }; > > -static int get_keycode(const char *key) > +int get_key_index(const char *key) > { This should be moved to hmp.c instead of making it public. > - const KeyDef *p; > + int i, keycode; > char *endp; > - int ret; > + const KeyDef *p; > > - for(p = key_defs; p->name != NULL; p++) { > - if (!strcmp(key, p->name)) > - return p->keycode; > + for (i = 0; KeyCodes_lookup[i] != NULL; i++) { > + if (!strcmp(key, KeyCodes_lookup[i])) > + return i; > } > + > if (strstart(key, "0x", NULL)) { > - ret = strtoul(key, &endp, 0); > - if (*endp == '\0' && ret >= 0x01 && ret <= 0xff) > - return ret; > + keycode = strtoul(key, &endp, 0); > + if (*endp == '\0' && keycode >= 0x01 && keycode <= 0xff) > + i = 0; > + for (p = key_defs; p->name != NULL; p++) { > + if (keycode == p->keycode) > + return i; > + i++; > + } > } > + > 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; > > - while (nb_pending_keycodes > 0) { > - nb_pending_keycodes--; > - keycode = keycodes[nb_pending_keycodes]; > + while (keycodes != NULL) { > + if (keycodes->value > sizeof(key_defs) / sizeof(*(key_defs))) { I don't think you need this check, see my comment on qmp_sendkey(). > + keycodes = NULL; > + break; > + } > + > + keycode = key_defs[keycodes->value].keycode; I'm not sure I like this, as key_defs[] and the KeyCodes enum will have to be kept in sync and this can silently break. We could do: static const KeyDef key_defs[] = { [KEY_CODES_SHIFT] = { .keycode = 0x2a, .name = "shift" }, }; instead, to link the two tables. However, after this patch 'name' seems only to be used by the monitor completion code in monitor_find_completion(). So, maybe we could use KeyCodes_lookup[] there and drop 'name' and have an integer array to map the Keycodes enum to the hex values. > if (keycode & 0x80) > kbd_put_keycode(0xe0); > kbd_put_keycode(keycode | 0x80); > + > + keycodes = keycodes->next; > } > } > > -static void do_sendkey(Monitor *mon, const QDict *qdict) > +void qmp_sendkey(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]; > > - if (nb_pending_keycodes > 0) { > + if (keycodes != NULL) { > qemu_del_timer(key_timer); > release_keys(NULL); > } > if (!has_hold_time) > hold_time = 100; > - 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; > - } > > - 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; > + keycodes = keys; > + while (keycodes != NULL) { I think that using a for loop is better. > + if (keycodes->value > sizeof(key_defs) / sizeof(*(key_defs))) { > + sprintf(value, "%d", keycodes->value); > + error_set(errp, QERR_INVALID_PARAMETER, value); > + return; > } This is not necessary. The qapi will do this check, qmp_sendkey() won't be called if there's an error. Actually, our error message is buggy right now, I'll submit a patch. > - if (!separator) > - break; > - keys = separator + 1; > - } > - nb_pending_keycodes = i; > - /* key down events */ > - for (i = 0; i < nb_pending_keycodes; i++) { > - keycode = keycodes[i]; > + > + /* key down events */ > + keycode = key_defs[keycodes->value].keycode; > if (keycode & 0x80) > kbd_put_keycode(0xe0); > kbd_put_keycode(keycode & 0x7f); > + > + keycodes = keycodes->next; > } > + keycodes = keys; > + > /* delayed key up events */ > qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) + > muldiv64(get_ticks_per_sec(), hold_time, 1000)); > diff --git a/monitor.h b/monitor.h > index 5f4de1b..1723622 100644 > --- a/monitor.h > +++ b/monitor.h > @@ -86,4 +86,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret); > > int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret); > > +int get_key_index(const char *key); > + > #endif /* !MONITOR_H */ > diff --git a/qapi-schema.json b/qapi-schema.json > index 3b6e346..5717467 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1862,3 +1862,48 @@ > # Since: 0.14.0 > ## > { 'command': 'netdev_del', 'data': {'id': 'str'} } > + > +## > +# @KeyCodes: > +# > +# An enumeration of key name. > +# > +# This is used by the sendkey command. > +# > +# Since: 1.2 > +## > +{ 'enum': 'KeyCodes', > + 'data': [ 'shift', 'shift_r', 'alt', 'alt_r', 'altgr', 'altgr_r', 'ctrl', > + 'ctrl_r', 'menu', 'esc', '1', '2', '3', '4', '5', '6', '7', '8', > + '9', '0', 'minus', 'equal', 'backspace', 'tab', 'q', 'w', 'e', > + 'r', 't', 'y', 'u', 'i', 'o', 'p', 'bracket_left', 'bracket_right', > + 'ret', 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'semicolon', > + 'apostrophe', 'grave_accent', 'backslash', 'z', 'x', 'c', 'v', 'b', > + 'n', 'm', 'comma', 'dot', 'slash', 'asterisk', 'spc', 'caps_lock', > + 'f1', 'f2', 'f3', 'f4', 'f5', 'f6', 'f7', 'f8', 'f9', 'f10', > + 'num_lock', 'scroll_lock', 'kp_divide', 'kp_multiply', > + 'kp_subtract', 'kp_add', 'kp_enter', 'kp_decimal', 'sysrq', 'kp_0', > + 'kp_1', 'kp_2', 'kp_3', 'kp_4', 'kp_5', 'kp_6', 'kp_7', 'kp_8', > + 'kp_9', 'less', 'f11', 'f12', 'print', 'home', 'pgup', 'pgdn', 'end', > + 'left', 'up', 'down', 'right', 'insert', 'delete', 'stop', 'again', > + 'props', 'undo', 'front', 'copy', 'open', 'paste', 'find', 'cut', > + 'lf', 'help', 'meta_l', 'meta_r', 'compose' ] } > + > +## > +# @sendkey: > +# > +# Send keys to VM. > +# > +# @keys: key sequence > +# @hold-time: time to delay key up events, milliseconds Please, say that hold-time is optional (look in this file for examples) and mention its default value. > +# > +# Returns: Nothing on success > +# If key is unknown or redundant, QERR_INVALID_PARAMETER How a key is redundant? And please do s/QERR_INVALID_PARAMETER/InvalidParameter > +# > +# Notes: Send keys to the guest. 'keys' could be the name of the > +# key. Use a JSON array to press several keys simultaneously. This should be part of the command's description, not a bottom note. > +# > +# Since: 1.2 > +## > +{ 'command': 'sendkey', > + 'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } } > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 2e1a38e..bcc6c4b 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -335,6 +335,33 @@ Example: > EQMP > > { > + .name = "sendkey", > + .args_type = "keys:O,hold-time:i?", > + .mhandler.cmd_new = qmp_marshal_input_sendkey, > + }, > + > +SQMP > +sendkey > +---------- > + > +Send keys to VM. > + > +Arguments: > + > +keys array: > + - "key": key sequence (json-string) > + > +- hold-time: time to delay key up events, milliseconds (josn-int, optional) > + > +Example: > + > +-> { "execute": "sendkey", > + "arguments": { 'keys': [ 'ctrl', 'alt', 'delete' ] } } > +<- { "return": {} } > + > +EQMP > + > + { > .name = "cpu", > .args_type = "index:i", > .mhandler.cmd_new = qmp_marshal_input_cpu,
On Wed, 20 Jun 2012 06:53:40 -0600 Eric Blake <eblake@redhat.com> wrote: > On 06/19/2012 10:47 PM, 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 monitor.c is the mapping of key name to keycode, > > Keys' order in the enmu and key_defs[] is same. > > > > The interface looks like we have settled into something useful that > libvirt can live with. I will leave a more thorough review of the > series to those more familiar with qemu's internals. However, as long > as we are fine-tuning things, I have some questions that might be worth > one more spin: > > > +## > > +# @sendkey: > > Should this be named 'send-key', given that most QMP commands separate > words with '-' rather than squasheverythingtogether? Yes, that's right. > > > +SQMP > > +sendkey > > +---------- > > + > > +Send keys to VM. > > + > > +Arguments: > > + > > +keys array: > > + - "key": key sequence (json-string) > > I'm not sure if json-string is the right designation any more. Rather, > it is a json-array of key enum values. > > > + > > +- hold-time: time to delay key up events, milliseconds (josn-int, optional) > > s/josn/json/ >
On 27/06/12 04:22, Luiz Capitulino wrote: > On Wed, 20 Jun 2012 06:53:40 -0600 > Eric Blake<eblake@redhat.com> wrote: > >> On 06/19/2012 10:47 PM, 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 monitor.c is the mapping of key name to keycode, >>> Keys' order in the enmu and key_defs[] is same. >>> >> >> The interface looks like we have settled into something useful that >> libvirt can live with. I will leave a more thorough review of the >> series to those more familiar with qemu's internals. However, as long >> as we are fine-tuning things, I have some questions that might be worth >> one more spin: >> >>> +## >>> +# @sendkey: >> >> Should this be named 'send-key', given that most QMP commands separate >> words with '-' rather than squasheverythingtogether? > > Yes, that's right. still use sendkey for hmp ? >>> +SQMP >>> +sendkey >>> +---------- >>> + >>> +Send keys to VM. >>> + >>> +Arguments: >>> + >>> +keys array: >>> + - "key": key sequence (json-string) >> >> I'm not sure if json-string is the right designation any more. Rather, >> it is a json-array of key enum values. The content in brackets should be the description of "key", which is the item of array. I think jaso-string is better. It's not the description for while array. >>> + >>> +- hold-time: time to delay key up events, milliseconds (josn-int, optional) >> >> s/josn/json/ Thanks.
On 27/06/12 04:21, Luiz Capitulino wrote: > On Wed, 20 Jun 2012 12:47:40 +0800 > Amos Kong<akong@redhat.com> 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 monitor.c is the mapping of key name to keycode, >> Keys' order in the enmu and key_defs[] is same. >> >> Signed-off-by: Amos Kong<akong@redhat.com> >> --- >> hmp-commands.hx | 2 +- >> hmp.c | 45 ++++++++++++++++++++++++ >> hmp.h | 1 + >> monitor.c | 102 +++++++++++++++++++++++------------------------------ >> monitor.h | 2 + >> qapi-schema.json | 45 ++++++++++++++++++++++++ >> qmp-commands.hx | 27 ++++++++++++++ >> 7 files changed, 165 insertions(+), 59 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index e336251..fee4c14 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_sendkey, >> }, >> >> STEXI >> diff --git a/hmp.c b/hmp.c >> index b9cec1d..846e8b9 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -19,6 +19,7 @@ >> #include "qemu-timer.h" >> #include "qmp-commands.h" >> #include "monitor.h" >> +#include "qapi-types.h" >> >> static void hmp_handle_error(Monitor *mon, Error **errp) >> { >> @@ -1000,3 +1001,47 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) >> qmp_netdev_del(id,&err); >> hmp_handle_error(mon,&err); >> } >> + >> +void hmp_sendkey(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]; >> + > > Please, drop this blank line. > >> + 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); >> + keylist->next = NULL; >> + >> + if (tmp) >> + tmp->next = keylist; >> + tmp = keylist; >> + if (!head) >> + head = keylist; >> + >> + if (!separator) >> + break; >> + keys = separator + 1; >> + } >> + >> + qmp_sendkey(head, has_hold_time, hold_time,&err); >> + hmp_handle_error(mon,&err); >> + qapi_free_KeyCodesList(head); >> +} >> diff --git a/hmp.h b/hmp.h >> index 79d138d..bcc74d2 100644 >> --- a/hmp.h >> +++ b/hmp.h >> @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); >> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); >> void hmp_netdev_add(Monitor *mon, const QDict *qdict); >> void hmp_netdev_del(Monitor *mon, const QDict *qdict); >> +void hmp_sendkey(Monitor *mon, const QDict *qdict); >> >> #endif >> diff --git a/monitor.c b/monitor.c >> index 6fa0104..863c0c6 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -1470,98 +1470,84 @@ static const KeyDef key_defs[] = { >> { 0, NULL }, >> }; >> >> -static int get_keycode(const char *key) >> +int get_key_index(const char *key) >> { > > This should be moved to hmp.c instead of making it public. The static 'key_defs' is used in this function, I tried to move key_defs definition to monitor.h, but I got this error: qemu/monitor.h:220:13: error: attempt to use poisoned "TARGET_SPARC" qemu/monitor.h:220:39: error: attempt to use poisoned "TARGET_SPARC64" Where is the good place to define key_defs ? it would be used in monitor.c & hmp.c >> - const KeyDef *p; >> + int i, keycode; >> char *endp; >> - int ret; >> + const KeyDef *p; >> >> - for(p = key_defs; p->name != NULL; p++) { >> - if (!strcmp(key, p->name)) >> - return p->keycode; >> + for (i = 0; KeyCodes_lookup[i] != NULL; i++) { >> + if (!strcmp(key, KeyCodes_lookup[i])) >> + return i; >> } >> + >> if (strstart(key, "0x", NULL)) { >> - ret = strtoul(key,&endp, 0); >> - if (*endp == '\0'&& ret>= 0x01&& ret<= 0xff) >> - return ret; >> + keycode = strtoul(key,&endp, 0); >> + if (*endp == '\0'&& keycode>= 0x01&& keycode<= 0xff) >> + i = 0; >> + for (p = key_defs; p->name != NULL; p++) { >> + if (keycode == p->keycode) >> + return i; >> + i++; >> + } >> } >> + >> 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; >> >> - while (nb_pending_keycodes> 0) { >> - nb_pending_keycodes--; >> - keycode = keycodes[nb_pending_keycodes]; >> + while (keycodes != NULL) { >> + if (keycodes->value> sizeof(key_defs) / sizeof(*(key_defs))) { > > I don't think you need this check, see my comment on qmp_sendkey(). > >> + keycodes = NULL; >> + break; >> + } >> + >> + keycode = key_defs[keycodes->value].keycode; > > I'm not sure I like this, as key_defs[] and the KeyCodes enum will have to be > kept in sync and this can silently break. We could do: > > static const KeyDef key_defs[] = { > [KEY_CODES_SHIFT] = { .keycode = 0x2a, .name = "shift" }, > }; > > instead, to link the two tables. > > However, after this patch 'name' seems only to be used by the monitor completion > code in monitor_find_completion(). So, maybe we could use KeyCodes_lookup[] there > and drop 'name' and have an integer array to map the Keycodes enum to the hex > values. I'll have a try. >> if (keycode& 0x80) >> kbd_put_keycode(0xe0); >> kbd_put_keycode(keycode | 0x80); >> + >> + keycodes = keycodes->next; >> } >> } >> >> -static void do_sendkey(Monitor *mon, const QDict *qdict) >> +void qmp_sendkey(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]; >> >> - if (nb_pending_keycodes> 0) { >> + if (keycodes != NULL) { >> qemu_del_timer(key_timer); >> release_keys(NULL); >> } >> if (!has_hold_time) >> hold_time = 100; >> - 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; >> - } >> >> - 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; >> + keycodes = keys; >> + while (keycodes != NULL) { > > I think that using a for loop is better. >> + if (keycodes->value> sizeof(key_defs) / sizeof(*(key_defs))) { >> + sprintf(value, "%d", keycodes->value); >> + error_set(errp, QERR_INVALID_PARAMETER, value); >> + return; >> } > > This is not necessary. The qapi will do this check, qmp_sendkey() won't be > called if there's an error. For qmp command, it will only pass valid keycode. but hmp_sendkey() might pass invalid parameters. Or we should check this in hmp_sendkey(). > Actually, our error message is buggy right now, I'll submit a patch. > >> - if (!separator) >> - break; >> - keys = separator + 1; >> - } >> - nb_pending_keycodes = i; >> - /* key down events */ >> - for (i = 0; i< nb_pending_keycodes; i++) { >> - keycode = keycodes[i]; >> + >> + /* key down events */ >> + keycode = key_defs[keycodes->value].keycode; >> if (keycode& 0x80) >> kbd_put_keycode(0xe0); >> kbd_put_keycode(keycode& 0x7f); >> + >> + keycodes = keycodes->next; >> } >> + keycodes = keys; >> + >> /* delayed key up events */ >> qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) + >> muldiv64(get_ticks_per_sec(), hold_time, 1000)); >> diff --git a/monitor.h b/monitor.h >> index 5f4de1b..1723622 100644 >> --- a/monitor.h >> +++ b/monitor.h >> @@ -86,4 +86,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret); >> >> int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret); >> >> +int get_key_index(const char *key); >> + >> #endif /* !MONITOR_H */ >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 3b6e346..5717467 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -1862,3 +1862,48 @@ >> # Since: 0.14.0 >> ## >> { 'command': 'netdev_del', 'data': {'id': 'str'} } >> + >> +## >> +# @KeyCodes: >> +# >> +# An enumeration of key name. >> +# >> +# This is used by the sendkey command. >> +# >> +# Since: 1.2 >> +## >> +{ 'enum': 'KeyCodes', >> + 'data': [ 'shift', 'shift_r', 'alt', 'alt_r', 'altgr', 'altgr_r', 'ctrl', >> + 'ctrl_r', 'menu', 'esc', '1', '2', '3', '4', '5', '6', '7', '8', >> + '9', '0', 'minus', 'equal', 'backspace', 'tab', 'q', 'w', 'e', >> + 'r', 't', 'y', 'u', 'i', 'o', 'p', 'bracket_left', 'bracket_right', >> + 'ret', 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'semicolon', >> + 'apostrophe', 'grave_accent', 'backslash', 'z', 'x', 'c', 'v', 'b', >> + 'n', 'm', 'comma', 'dot', 'slash', 'asterisk', 'spc', 'caps_lock', >> + 'f1', 'f2', 'f3', 'f4', 'f5', 'f6', 'f7', 'f8', 'f9', 'f10', >> + 'num_lock', 'scroll_lock', 'kp_divide', 'kp_multiply', >> + 'kp_subtract', 'kp_add', 'kp_enter', 'kp_decimal', 'sysrq', 'kp_0', >> + 'kp_1', 'kp_2', 'kp_3', 'kp_4', 'kp_5', 'kp_6', 'kp_7', 'kp_8', >> + 'kp_9', 'less', 'f11', 'f12', 'print', 'home', 'pgup', 'pgdn', 'end', >> + 'left', 'up', 'down', 'right', 'insert', 'delete', 'stop', 'again', >> + 'props', 'undo', 'front', 'copy', 'open', 'paste', 'find', 'cut', >> + 'lf', 'help', 'meta_l', 'meta_r', 'compose' ] } >> + >> +## >> +# @sendkey: >> +# >> +# Send keys to VM. >> +# >> +# @keys: key sequence > >> +# @hold-time: time to delay key up events, milliseconds > > Please, say that hold-time is optional (look in this file for examples) and > mention its default value. Got it. >> +# >> +# Returns: Nothing on success >> +# If key is unknown or redundant, QERR_INVALID_PARAMETER > > How a key is redundant? And please do s/QERR_INVALID_PARAMETER/InvalidParameter 16 limitation had been removed, the 'redundant' also needs to be removed. >> +# >> +# Notes: Send keys to the guest. 'keys' could be the name of the >> +# key. Use a JSON array to press several keys simultaneously. > > This should be part of the command's description, not a bottom note. >> +# >> +# Since: 1.2 >> +## >> +{ 'command': 'sendkey', >> + 'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } } >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 2e1a38e..bcc6c4b 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -335,6 +335,33 @@ Example: >> EQMP >> >> { >> + .name = "sendkey", >> + .args_type = "keys:O,hold-time:i?", >> + .mhandler.cmd_new = qmp_marshal_input_sendkey, >> + }, >> + >> +SQMP >> +sendkey >> +---------- >> + >> +Send keys to VM. >> + >> +Arguments: >> + >> +keys array: >> + - "key": key sequence (json-string) >> + >> +- hold-time: time to delay key up events, milliseconds (josn-int, optional) >> + >> +Example: >> + >> +-> { "execute": "sendkey", >> + "arguments": { 'keys': [ 'ctrl', 'alt', 'delete' ] } } >> +<- { "return": {} } >> + >> +EQMP >> + >> + { >> .name = "cpu", >> .args_type = "index:i", >> .mhandler.cmd_new = qmp_marshal_input_cpu, Thanks for your time.
On 06/27/2012 03:59 AM, Amos Kong wrote: >>> The interface looks like we have settled into something useful that >>> libvirt can live with. I will leave a more thorough review of the >>> series to those more familiar with qemu's internals. However, as long >>> as we are fine-tuning things, I have some questions that might be worth >>> one more spin: >>> >>>> +## >>>> +# @sendkey: >>> >>> Should this be named 'send-key', given that most QMP commands separate >>> words with '-' rather than squasheverythingtogether? >> >> Yes, that's right. > > still use sendkey for hmp ? Libvirt won't care if you rename the hmp command, once the QMP command is in place. However, for back-compat, since hmp already had 'sendkey', it probably makes sense to keep that naming instead of gratuitously changing it, so for now, I'd go with 'send-key' only for QMP.
On Wed, 27 Jun 2012 17:59:04 +0800 Amos Kong <akong@redhat.com> wrote: > On 27/06/12 04:22, Luiz Capitulino wrote: > > On Wed, 20 Jun 2012 06:53:40 -0600 > > Eric Blake<eblake@redhat.com> wrote: > > > >> On 06/19/2012 10:47 PM, 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 monitor.c is the mapping of key name to keycode, > >>> Keys' order in the enmu and key_defs[] is same. > >>> > >> > >> The interface looks like we have settled into something useful that > >> libvirt can live with. I will leave a more thorough review of the > >> series to those more familiar with qemu's internals. However, as long > >> as we are fine-tuning things, I have some questions that might be worth > >> one more spin: > >> > >>> +## > >>> +# @sendkey: > >> > >> Should this be named 'send-key', given that most QMP commands separate > >> words with '-' rather than squasheverythingtogether? > > > > Yes, that's right. > > still use sendkey for hmp ? Yes. > >>> +SQMP > >>> +sendkey > >>> +---------- > >>> + > >>> +Send keys to VM. > >>> + > >>> +Arguments: > >>> + > >>> +keys array: > >>> + - "key": key sequence (json-string) > >> > >> I'm not sure if json-string is the right designation any more. Rather, > >> it is a json-array of key enum values. > > The content in brackets should be the description of "key", > which is the item of array. I think jaso-string is better. > It's not the description for while array. Actually, you should call it a list of json-string.
On Wed, 27 Jun 2012 18:22:48 +0800 Amos Kong <akong@redhat.com> wrote: > On 27/06/12 04:21, Luiz Capitulino wrote: > > On Wed, 20 Jun 2012 12:47:40 +0800 > > Amos Kong<akong@redhat.com> 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 monitor.c is the mapping of key name to keycode, > >> Keys' order in the enmu and key_defs[] is same. > >> > >> Signed-off-by: Amos Kong<akong@redhat.com> > >> --- > >> hmp-commands.hx | 2 +- > >> hmp.c | 45 ++++++++++++++++++++++++ > >> hmp.h | 1 + > >> monitor.c | 102 +++++++++++++++++++++++------------------------------ > >> monitor.h | 2 + > >> qapi-schema.json | 45 ++++++++++++++++++++++++ > >> qmp-commands.hx | 27 ++++++++++++++ > >> 7 files changed, 165 insertions(+), 59 deletions(-) > >> > >> diff --git a/hmp-commands.hx b/hmp-commands.hx > >> index e336251..fee4c14 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_sendkey, > >> }, > >> > >> STEXI > >> diff --git a/hmp.c b/hmp.c > >> index b9cec1d..846e8b9 100644 > >> --- a/hmp.c > >> +++ b/hmp.c > >> @@ -19,6 +19,7 @@ > >> #include "qemu-timer.h" > >> #include "qmp-commands.h" > >> #include "monitor.h" > >> +#include "qapi-types.h" > >> > >> static void hmp_handle_error(Monitor *mon, Error **errp) > >> { > >> @@ -1000,3 +1001,47 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) > >> qmp_netdev_del(id,&err); > >> hmp_handle_error(mon,&err); > >> } > >> + > >> +void hmp_sendkey(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]; > >> + > > > > Please, drop this blank line. > > > >> + 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); > >> + keylist->next = NULL; > >> + > >> + if (tmp) > >> + tmp->next = keylist; > >> + tmp = keylist; > >> + if (!head) > >> + head = keylist; > >> + > >> + if (!separator) > >> + break; > >> + keys = separator + 1; > >> + } > >> + > >> + qmp_sendkey(head, has_hold_time, hold_time,&err); > >> + hmp_handle_error(mon,&err); > >> + qapi_free_KeyCodesList(head); > >> +} > >> diff --git a/hmp.h b/hmp.h > >> index 79d138d..bcc74d2 100644 > >> --- a/hmp.h > >> +++ b/hmp.h > >> @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); > >> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); > >> void hmp_netdev_add(Monitor *mon, const QDict *qdict); > >> void hmp_netdev_del(Monitor *mon, const QDict *qdict); > >> +void hmp_sendkey(Monitor *mon, const QDict *qdict); > >> > >> #endif > >> diff --git a/monitor.c b/monitor.c > >> index 6fa0104..863c0c6 100644 > >> --- a/monitor.c > >> +++ b/monitor.c > >> @@ -1470,98 +1470,84 @@ static const KeyDef key_defs[] = { > >> { 0, NULL }, > >> }; > >> > >> -static int get_keycode(const char *key) > >> +int get_key_index(const char *key) > >> { > > > > This should be moved to hmp.c instead of making it public. > > > The static 'key_defs' is used in this function, > I tried to move key_defs definition to monitor.h, > but I got this error: > > qemu/monitor.h:220:13: error: attempt to use poisoned "TARGET_SPARC" > qemu/monitor.h:220:39: error: attempt to use poisoned "TARGET_SPARC64" > > Where is the good place to define key_defs ? it would be used in > monitor.c & hmp.c You could move it to input.c (whose functions are declared in console.h). Actually, qmp_sendkey() should be moved there too. But I'm now wondering if we're doing it right. The HMP interface is supposed to be a real QMP client. However, to support hex keycodes it has to have knowledge of the key mapping. Having access to key_defs if cheating, as hmp.c is internal to qemu. The two solutions I can think of are adding query-keymap or change send-key to also accept hex values (which could be done through a new command). Ideas? > >> - const KeyDef *p; > >> + int i, keycode; > >> char *endp; > >> - int ret; > >> + const KeyDef *p; > >> > >> - for(p = key_defs; p->name != NULL; p++) { > >> - if (!strcmp(key, p->name)) > >> - return p->keycode; > >> + for (i = 0; KeyCodes_lookup[i] != NULL; i++) { > >> + if (!strcmp(key, KeyCodes_lookup[i])) > >> + return i; > >> } > >> + > >> if (strstart(key, "0x", NULL)) { > >> - ret = strtoul(key,&endp, 0); > >> - if (*endp == '\0'&& ret>= 0x01&& ret<= 0xff) > >> - return ret; > >> + keycode = strtoul(key,&endp, 0); > >> + if (*endp == '\0'&& keycode>= 0x01&& keycode<= 0xff) > >> + i = 0; > >> + for (p = key_defs; p->name != NULL; p++) { > >> + if (keycode == p->keycode) > >> + return i; > >> + i++; > >> + } > >> } > >> + > >> 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; > >> > >> - while (nb_pending_keycodes> 0) { > >> - nb_pending_keycodes--; > >> - keycode = keycodes[nb_pending_keycodes]; > >> + while (keycodes != NULL) { > >> + if (keycodes->value> sizeof(key_defs) / sizeof(*(key_defs))) { > > > > I don't think you need this check, see my comment on qmp_sendkey(). > > > > > >> + keycodes = NULL; > >> + break; > >> + } > >> + > >> + keycode = key_defs[keycodes->value].keycode; > > > > I'm not sure I like this, as key_defs[] and the KeyCodes enum will have to be > > kept in sync and this can silently break. We could do: > > > > static const KeyDef key_defs[] = { > > [KEY_CODES_SHIFT] = { .keycode = 0x2a, .name = "shift" }, > > }; > > > > instead, to link the two tables. > > > > However, after this patch 'name' seems only to be used by the monitor completion > > code in monitor_find_completion(). So, maybe we could use KeyCodes_lookup[] there > > and drop 'name' and have an integer array to map the Keycodes enum to the hex > > values. > > I'll have a try. > > > >> if (keycode& 0x80) > >> kbd_put_keycode(0xe0); > >> kbd_put_keycode(keycode | 0x80); > >> + > >> + keycodes = keycodes->next; > >> } > >> } > >> > >> -static void do_sendkey(Monitor *mon, const QDict *qdict) > >> +void qmp_sendkey(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]; > >> > >> - if (nb_pending_keycodes> 0) { > >> + if (keycodes != NULL) { > >> qemu_del_timer(key_timer); > >> release_keys(NULL); > >> } > >> if (!has_hold_time) > >> hold_time = 100; > >> - 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; > >> - } > >> > >> - 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; > >> + keycodes = keys; > >> + while (keycodes != NULL) { > > > > I think that using a for loop is better. > > > >> + if (keycodes->value> sizeof(key_defs) / sizeof(*(key_defs))) { > >> + sprintf(value, "%d", keycodes->value); > >> + error_set(errp, QERR_INVALID_PARAMETER, value); > >> + return; > >> } > > > > This is not necessary. The qapi will do this check, qmp_sendkey() won't be > > called if there's an error. > > For qmp command, it will only pass valid keycode. but hmp_sendkey() > might pass > invalid parameters. Or we should check this in hmp_sendkey(). Good point, but then please use KEY_CODE_MAX. > > > > Actually, our error message is buggy right now, I'll submit a patch. > > > >> - if (!separator) > >> - break; > >> - keys = separator + 1; > >> - } > >> - nb_pending_keycodes = i; > >> - /* key down events */ > >> - for (i = 0; i< nb_pending_keycodes; i++) { > >> - keycode = keycodes[i]; > >> + > >> + /* key down events */ > >> + keycode = key_defs[keycodes->value].keycode; > >> if (keycode& 0x80) > >> kbd_put_keycode(0xe0); > >> kbd_put_keycode(keycode& 0x7f); > >> + > >> + keycodes = keycodes->next; > >> } > >> + keycodes = keys; > >> + > >> /* delayed key up events */ > >> qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) + > >> muldiv64(get_ticks_per_sec(), hold_time, 1000)); > >> diff --git a/monitor.h b/monitor.h > >> index 5f4de1b..1723622 100644 > >> --- a/monitor.h > >> +++ b/monitor.h > >> @@ -86,4 +86,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret); > >> > >> int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret); > >> > >> +int get_key_index(const char *key); > >> + > >> #endif /* !MONITOR_H */ > >> diff --git a/qapi-schema.json b/qapi-schema.json > >> index 3b6e346..5717467 100644 > >> --- a/qapi-schema.json > >> +++ b/qapi-schema.json > >> @@ -1862,3 +1862,48 @@ > >> # Since: 0.14.0 > >> ## > >> { 'command': 'netdev_del', 'data': {'id': 'str'} } > >> + > >> +## > >> +# @KeyCodes: > >> +# > >> +# An enumeration of key name. > >> +# > >> +# This is used by the sendkey command. > >> +# > >> +# Since: 1.2 > >> +## > >> +{ 'enum': 'KeyCodes', > >> + 'data': [ 'shift', 'shift_r', 'alt', 'alt_r', 'altgr', 'altgr_r', 'ctrl', > >> + 'ctrl_r', 'menu', 'esc', '1', '2', '3', '4', '5', '6', '7', '8', > >> + '9', '0', 'minus', 'equal', 'backspace', 'tab', 'q', 'w', 'e', > >> + 'r', 't', 'y', 'u', 'i', 'o', 'p', 'bracket_left', 'bracket_right', > >> + 'ret', 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'semicolon', > >> + 'apostrophe', 'grave_accent', 'backslash', 'z', 'x', 'c', 'v', 'b', > >> + 'n', 'm', 'comma', 'dot', 'slash', 'asterisk', 'spc', 'caps_lock', > >> + 'f1', 'f2', 'f3', 'f4', 'f5', 'f6', 'f7', 'f8', 'f9', 'f10', > >> + 'num_lock', 'scroll_lock', 'kp_divide', 'kp_multiply', > >> + 'kp_subtract', 'kp_add', 'kp_enter', 'kp_decimal', 'sysrq', 'kp_0', > >> + 'kp_1', 'kp_2', 'kp_3', 'kp_4', 'kp_5', 'kp_6', 'kp_7', 'kp_8', > >> + 'kp_9', 'less', 'f11', 'f12', 'print', 'home', 'pgup', 'pgdn', 'end', > >> + 'left', 'up', 'down', 'right', 'insert', 'delete', 'stop', 'again', > >> + 'props', 'undo', 'front', 'copy', 'open', 'paste', 'find', 'cut', > >> + 'lf', 'help', 'meta_l', 'meta_r', 'compose' ] } > >> + > >> +## > >> +# @sendkey: > >> +# > >> +# Send keys to VM. > >> +# > >> +# @keys: key sequence > > > >> +# @hold-time: time to delay key up events, milliseconds > > > > Please, say that hold-time is optional (look in this file for examples) and > > mention its default value. > > Got it. > > >> +# > >> +# Returns: Nothing on success > >> +# If key is unknown or redundant, QERR_INVALID_PARAMETER > > > > How a key is redundant? And please do s/QERR_INVALID_PARAMETER/InvalidParameter > > > 16 limitation had been removed, the 'redundant' also needs to be removed. > > >> +# > >> +# Notes: Send keys to the guest. 'keys' could be the name of the > >> +# key. Use a JSON array to press several keys simultaneously. > > > > This should be part of the command's description, not a bottom note. > > > >> +# > >> +# Since: 1.2 > >> +## > >> +{ 'command': 'sendkey', > >> + 'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } } > >> diff --git a/qmp-commands.hx b/qmp-commands.hx > >> index 2e1a38e..bcc6c4b 100644 > >> --- a/qmp-commands.hx > >> +++ b/qmp-commands.hx > >> @@ -335,6 +335,33 @@ Example: > >> EQMP > >> > >> { > >> + .name = "sendkey", > >> + .args_type = "keys:O,hold-time:i?", > >> + .mhandler.cmd_new = qmp_marshal_input_sendkey, > >> + }, > >> + > >> +SQMP > >> +sendkey > >> +---------- > >> + > >> +Send keys to VM. > >> + > >> +Arguments: > >> + > >> +keys array: > >> + - "key": key sequence (json-string) > >> + > >> +- hold-time: time to delay key up events, milliseconds (josn-int, optional) > >> + > >> +Example: > >> + > >> +-> { "execute": "sendkey", > >> + "arguments": { 'keys': [ 'ctrl', 'alt', 'delete' ] } } > >> +<- { "return": {} } > >> + > >> +EQMP > >> + > >> + { > >> .name = "cpu", > >> .args_type = "index:i", > >> .mhandler.cmd_new = qmp_marshal_input_cpu, > > Thanks for your time. >
diff --git a/hmp-commands.hx b/hmp-commands.hx index e336251..fee4c14 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_sendkey, }, STEXI diff --git a/hmp.c b/hmp.c index b9cec1d..846e8b9 100644 --- a/hmp.c +++ b/hmp.c @@ -19,6 +19,7 @@ #include "qemu-timer.h" #include "qmp-commands.h" #include "monitor.h" +#include "qapi-types.h" static void hmp_handle_error(Monitor *mon, Error **errp) { @@ -1000,3 +1001,47 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) qmp_netdev_del(id, &err); hmp_handle_error(mon, &err); } + +void hmp_sendkey(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); + keylist->next = NULL; + + if (tmp) + tmp->next = keylist; + tmp = keylist; + if (!head) + head = keylist; + + if (!separator) + break; + keys = separator + 1; + } + + qmp_sendkey(head, has_hold_time, hold_time, &err); + hmp_handle_error(mon, &err); + qapi_free_KeyCodesList(head); +} diff --git a/hmp.h b/hmp.h index 79d138d..bcc74d2 100644 --- a/hmp.h +++ b/hmp.h @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); void hmp_netdev_add(Monitor *mon, const QDict *qdict); void hmp_netdev_del(Monitor *mon, const QDict *qdict); +void hmp_sendkey(Monitor *mon, const QDict *qdict); #endif diff --git a/monitor.c b/monitor.c index 6fa0104..863c0c6 100644 --- a/monitor.c +++ b/monitor.c @@ -1470,98 +1470,84 @@ static const KeyDef key_defs[] = { { 0, NULL }, }; -static int get_keycode(const char *key) +int get_key_index(const char *key) { - const KeyDef *p; + int i, keycode; char *endp; - int ret; + const KeyDef *p; - for(p = key_defs; p->name != NULL; p++) { - if (!strcmp(key, p->name)) - return p->keycode; + for (i = 0; KeyCodes_lookup[i] != NULL; i++) { + if (!strcmp(key, KeyCodes_lookup[i])) + return i; } + if (strstart(key, "0x", NULL)) { - ret = strtoul(key, &endp, 0); - if (*endp == '\0' && ret >= 0x01 && ret <= 0xff) - return ret; + keycode = strtoul(key, &endp, 0); + if (*endp == '\0' && keycode >= 0x01 && keycode <= 0xff) + i = 0; + for (p = key_defs; p->name != NULL; p++) { + if (keycode == p->keycode) + return i; + i++; + } } + 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; - while (nb_pending_keycodes > 0) { - nb_pending_keycodes--; - keycode = keycodes[nb_pending_keycodes]; + while (keycodes != NULL) { + if (keycodes->value > sizeof(key_defs) / sizeof(*(key_defs))) { + keycodes = NULL; + break; + } + + keycode = key_defs[keycodes->value].keycode; if (keycode & 0x80) kbd_put_keycode(0xe0); kbd_put_keycode(keycode | 0x80); + + keycodes = keycodes->next; } } -static void do_sendkey(Monitor *mon, const QDict *qdict) +void qmp_sendkey(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]; - if (nb_pending_keycodes > 0) { + if (keycodes != NULL) { qemu_del_timer(key_timer); release_keys(NULL); } if (!has_hold_time) hold_time = 100; - 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; - } - 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; + keycodes = keys; + while (keycodes != NULL) { + if (keycodes->value > sizeof(key_defs) / sizeof(*(key_defs))) { + sprintf(value, "%d", keycodes->value); + error_set(errp, QERR_INVALID_PARAMETER, value); + return; } - if (!separator) - break; - keys = separator + 1; - } - nb_pending_keycodes = i; - /* key down events */ - for (i = 0; i < nb_pending_keycodes; i++) { - keycode = keycodes[i]; + + /* key down events */ + keycode = key_defs[keycodes->value].keycode; if (keycode & 0x80) kbd_put_keycode(0xe0); kbd_put_keycode(keycode & 0x7f); + + keycodes = keycodes->next; } + keycodes = keys; + /* delayed key up events */ qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) + muldiv64(get_ticks_per_sec(), hold_time, 1000)); diff --git a/monitor.h b/monitor.h index 5f4de1b..1723622 100644 --- a/monitor.h +++ b/monitor.h @@ -86,4 +86,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret); int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret); +int get_key_index(const char *key); + #endif /* !MONITOR_H */ diff --git a/qapi-schema.json b/qapi-schema.json index 3b6e346..5717467 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1862,3 +1862,48 @@ # Since: 0.14.0 ## { 'command': 'netdev_del', 'data': {'id': 'str'} } + +## +# @KeyCodes: +# +# An enumeration of key name. +# +# This is used by the sendkey command. +# +# Since: 1.2 +## +{ 'enum': 'KeyCodes', + 'data': [ 'shift', 'shift_r', 'alt', 'alt_r', 'altgr', 'altgr_r', 'ctrl', + 'ctrl_r', 'menu', 'esc', '1', '2', '3', '4', '5', '6', '7', '8', + '9', '0', 'minus', 'equal', 'backspace', 'tab', 'q', 'w', 'e', + 'r', 't', 'y', 'u', 'i', 'o', 'p', 'bracket_left', 'bracket_right', + 'ret', 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'semicolon', + 'apostrophe', 'grave_accent', 'backslash', 'z', 'x', 'c', 'v', 'b', + 'n', 'm', 'comma', 'dot', 'slash', 'asterisk', 'spc', 'caps_lock', + 'f1', 'f2', 'f3', 'f4', 'f5', 'f6', 'f7', 'f8', 'f9', 'f10', + 'num_lock', 'scroll_lock', 'kp_divide', 'kp_multiply', + 'kp_subtract', 'kp_add', 'kp_enter', 'kp_decimal', 'sysrq', 'kp_0', + 'kp_1', 'kp_2', 'kp_3', 'kp_4', 'kp_5', 'kp_6', 'kp_7', 'kp_8', + 'kp_9', 'less', 'f11', 'f12', 'print', 'home', 'pgup', 'pgdn', 'end', + 'left', 'up', 'down', 'right', 'insert', 'delete', 'stop', 'again', + 'props', 'undo', 'front', 'copy', 'open', 'paste', 'find', 'cut', + 'lf', 'help', 'meta_l', 'meta_r', 'compose' ] } + +## +# @sendkey: +# +# Send keys to VM. +# +# @keys: key sequence +# @hold-time: time to delay key up events, milliseconds +# +# Returns: Nothing on success +# If key is unknown or redundant, QERR_INVALID_PARAMETER +# +# Notes: Send keys to the guest. 'keys' could be the name of the +# key. Use a JSON array to press several keys simultaneously. +# +# Since: 1.2 +## +{ 'command': 'sendkey', + 'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } } diff --git a/qmp-commands.hx b/qmp-commands.hx index 2e1a38e..bcc6c4b 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -335,6 +335,33 @@ Example: EQMP { + .name = "sendkey", + .args_type = "keys:O,hold-time:i?", + .mhandler.cmd_new = qmp_marshal_input_sendkey, + }, + +SQMP +sendkey +---------- + +Send keys to VM. + +Arguments: + +keys array: + - "key": key sequence (json-string) + +- hold-time: time to delay key up events, milliseconds (josn-int, optional) + +Example: + +-> { "execute": "sendkey", + "arguments": { 'keys': [ 'ctrl', 'alt', 'delete' ] } } +<- { "return": {} } + +EQMP + + { .name = "cpu", .args_type = "index:i", .mhandler.cmd_new = qmp_marshal_input_cpu,
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 monitor.c is the mapping of key name to keycode, Keys' order in the enmu and key_defs[] is same. Signed-off-by: Amos Kong <akong@redhat.com> --- hmp-commands.hx | 2 +- hmp.c | 45 ++++++++++++++++++++++++ hmp.h | 1 + monitor.c | 102 +++++++++++++++++++++++------------------------------ monitor.h | 2 + qapi-schema.json | 45 ++++++++++++++++++++++++ qmp-commands.hx | 27 ++++++++++++++ 7 files changed, 165 insertions(+), 59 deletions(-)