diff mbox

[v4,5/6] qapi: convert sendkey

Message ID 1341492525-29809-6-git-send-email-akong@redhat.com
State New
Headers show

Commit Message

Amos Kong July 5, 2012, 12:48 p.m. UTC
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 <akong@redhat.com>
---
 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(-)

Comments

Luiz Capitulino July 12, 2012, 3:09 p.m. UTC | #1
On Thu,  5 Jul 2012 20:48:44 +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 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 <akong@redhat.com>
> ---
>  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.

> +} 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()) and write the following
public functions:

 o KeyCode keycode_from_key(const char *key);
 o KeyCode keycode_from_code(int code);

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).

> +    [KEY_CODES_SHIFT] = { 0x2a, "shift" },
> +    [KEY_CODES_SHIFT_R] = { 0x36, "shift_r" },
> +
> +    [KEY_CODES_ALT] = { 0x38, "alt" },
> +    [KEY_CODES_ALT_R] = { 0xb8, "alt_r" },
> +    [KEY_CODES_ALTGR] = { 0x64, "altgr" },
> +    [KEY_CODES_ALTGR_R] = { 0xe4, "altgr_r" },
> +    [KEY_CODES_CTRL] = { 0x1d, "ctrl" },
> +    [KEY_CODES_CTRL_R] = { 0x9d, "ctrl_r" },
> +
> +    [KEY_CODES_MENU] = { 0xdd, "menu" },
> +
> +    [KEY_CODES_ESC] = { 0x01, "esc" },
> +
> +    [KEY_CODES_1] = { 0x02, "1" },
> +    [KEY_CODES_2] = { 0x03, "2" },
> +    [KEY_CODES_3] = { 0x04, "3" },
> +    [KEY_CODES_4] = { 0x05, "4" },
> +    [KEY_CODES_5] = { 0x06, "5" },
> +    [KEY_CODES_6] = { 0x07, "6" },
> +    [KEY_CODES_7] = { 0x08, "7" },
> +    [KEY_CODES_8] = { 0x09, "8" },
> +    [KEY_CODES_9] = { 0x0a, "9" },
> +    [KEY_CODES_0] = { 0x0b, "0" },
> +    [KEY_CODES_MINUS] = { 0x0c, "minus" },
> +    [KEY_CODES_EQUAL] = { 0x0d, "equal" },
> +    [KEY_CODES_BACKSPACE] = { 0x0e, "backspace" },
> +
> +    [KEY_CODES_TAB] = { 0x0f, "tab" },
> +    [KEY_CODES_Q] = { 0x10, "q" },
> +    [KEY_CODES_W] = { 0x11, "w" },
> +    [KEY_CODES_E] = { 0x12, "e" },
> +    [KEY_CODES_R] = { 0x13, "r" },
> +    [KEY_CODES_T] = { 0x14, "t" },
> +    [KEY_CODES_Y] = { 0x15, "y" },
> +    [KEY_CODES_U] = { 0x16, "u" },
> +    [KEY_CODES_I] = { 0x17, "i" },
> +    [KEY_CODES_O] = { 0x18, "o" },
> +    [KEY_CODES_P] = { 0x19, "p" },
> +    [KEY_CODES_BRACKET_LEFT] = { 0x1a, "bracket_left" },
> +    [KEY_CODES_BRACKET_RIGHT] = { 0x1b, "bracket_right" },
> +    [KEY_CODES_RET] = { 0x1c, "ret" },
> +
> +    [KEY_CODES_A] = { 0x1e, "a" },
> +    [KEY_CODES_S] = { 0x1f, "s" },
> +    [KEY_CODES_D] = { 0x20, "d" },
> +    [KEY_CODES_F] = { 0x21, "f" },
> +    [KEY_CODES_G] = { 0x22, "g" },
> +    [KEY_CODES_H] = { 0x23, "h" },
> +    [KEY_CODES_J] = { 0x24, "j" },
> +    [KEY_CODES_K] = { 0x25, "k" },
> +    [KEY_CODES_L] = { 0x26, "l" },
> +    [KEY_CODES_SEMICOLON] = { 0x27, "semicolon" },
> +    [KEY_CODES_APOSTROPHE] = { 0x28, "apostrophe" },
> +    [KEY_CODES_GRAVE_ACCENT] = { 0x29, "grave_accent" },
> +
> +    [KEY_CODES_BACKSLASH] = { 0x2b, "backslash" },
> +    [KEY_CODES_Z] = { 0x2c, "z" },
> +    [KEY_CODES_X] = { 0x2d, "x" },
> +    [KEY_CODES_C] = { 0x2e, "c" },
> +    [KEY_CODES_V] = { 0x2f, "v" },
> +    [KEY_CODES_B] = { 0x30, "b" },
> +    [KEY_CODES_N] = { 0x31, "n" },
> +    [KEY_CODES_M] = { 0x32, "m" },
> +    [KEY_CODES_COMMA] = { 0x33, "comma" },
> +    [KEY_CODES_DOT] = { 0x34, "dot" },
> +    [KEY_CODES_SLASH] = { 0x35, "slash" },
> +
> +    [KEY_CODES_ASTERISK] = { 0x37, "asterisk" },
> +
> +    [KEY_CODES_SPC] = { 0x39, "spc" },
> +    [KEY_CODES_CAPS_LOCK] = { 0x3a, "caps_lock" },
> +    [KEY_CODES_F1] = { 0x3b, "f1" },
> +    [KEY_CODES_F2] = { 0x3c, "f2" },
> +    [KEY_CODES_F3] = { 0x3d, "f3" },
> +    [KEY_CODES_F4] = { 0x3e, "f4" },
> +    [KEY_CODES_F5] = { 0x3f, "f5" },
> +    [KEY_CODES_F6] = { 0x40, "f6" },
> +    [KEY_CODES_F7] = { 0x41, "f7" },
> +    [KEY_CODES_F8] = { 0x42, "f8" },
> +    [KEY_CODES_F9] = { 0x43, "f9" },
> +    [KEY_CODES_F10] = { 0x44, "f10" },
> +    [KEY_CODES_NUM_LOCK] = { 0x45, "num_lock" },
> +    [KEY_CODES_SCROLL_LOCK] = { 0x46, "scroll_lock" },
> +
> +    [KEY_CODES_KP_DIVIDE] = { 0xb5, "kp_divide" },
> +    [KEY_CODES_KP_MULTIPLY] = { 0x37, "kp_multiply" },
> +    [KEY_CODES_KP_SUBTRACT] = { 0x4a, "kp_subtract" },
> +    [KEY_CODES_KP_ADD] = { 0x4e, "kp_add" },
> +    [KEY_CODES_KP_ENTER] = { 0x9c, "kp_enter" },
> +    [KEY_CODES_KP_DECIMAL] = { 0x53, "kp_decimal" },
> +    [KEY_CODES_SYSRQ] = { 0x54, "sysrq" },
> +
> +    [KEY_CODES_KP_0] = { 0x52, "kp_0" },
> +    [KEY_CODES_KP_1] = { 0x4f, "kp_1" },
> +    [KEY_CODES_KP_2] = { 0x50, "kp_2" },
> +    [KEY_CODES_KP_3] = { 0x51, "kp_3" },
> +    [KEY_CODES_KP_4] = { 0x4b, "kp_4" },
> +    [KEY_CODES_KP_5] = { 0x4c, "kp_5" },
> +    [KEY_CODES_KP_6] = { 0x4d, "kp_6" },
> +    [KEY_CODES_KP_7] = { 0x47, "kp_7" },
> +    [KEY_CODES_KP_8] = { 0x48, "kp_8" },
> +    [KEY_CODES_KP_9] = { 0x49, "kp_9" },
> +
> +    [KEY_CODES_LESS] = { 0x56, "less" },
> +
> +    [KEY_CODES_F11] = { 0x57, "f11" },
> +    [KEY_CODES_F12] = { 0x58, "f12" },
> +
> +    [KEY_CODES_PRINT] = { 0xb7, "print" },
> +
> +    [KEY_CODES_HOME] = { 0xc7, "home" },
> +    [KEY_CODES_PGUP] = { 0xc9, "pgup" },
> +    [KEY_CODES_PGDN] = { 0xd1, "pgdn" },
> +    [KEY_CODES_END] = { 0xcf, "end" },
> +
> +    [KEY_CODES_LEFT] = { 0xcb, "left" },
> +    [KEY_CODES_UP] = { 0xc8, "up" },
> +    [KEY_CODES_DOWN] = { 0xd0, "down" },
> +    [KEY_CODES_RIGHT] = { 0xcd, "right" },
> +
> +    [KEY_CODES_INSERT] = { 0xd2, "insert" },
> +    [KEY_CODES_DELETE] = { 0xd3, "delete" },
> +#ifdef NEED_CPU_H
> +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64)
> +    [KEY_CODES_STOP] = { 0xf0, "stop" },
> +    [KEY_CODES_AGAIN] = { 0xf1, "again" },
> +    [KEY_CODES_PROPS] = { 0xf2, "props" },
> +    [KEY_CODES_UNDO] = { 0xf3, "undo" },
> +    [KEY_CODES_FRONT] = { 0xf4, "front" },
> +    [KEY_CODES_COPY] = { 0xf5, "copy" },
> +    [KEY_CODES_OPEN] = { 0xf6, "open" },
> +    [KEY_CODES_PASTE] = { 0xf7, "paste" },
> +    [KEY_CODES_FIND] = { 0xf8, "find" },
> +    [KEY_CODES_CUT] = { 0xf9, "cut" },
> +    [KEY_CODES_LF] = { 0xfa, "lf" },
> +    [KEY_CODES_HELP] = { 0xfb, "help" },
> +    [KEY_CODES_META_L] = { 0xfc, "meta_l" },
> +    [KEY_CODES_META_R] = { 0xfd, "meta_r" },
> +    [KEY_CODES_COMPOSE] = { 0xfe, "compose" },
> +#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.

> +        keylist->next = NULL;
> +
> +        if (tmp)
> +            tmp->next = keylist;
> +        tmp = keylist;
> +        if (!head)
> +            head = keylist;
> +
> +        if (!separator)
> +            break;
> +        keys = separator + 1;
> +    }
> +
> +    qmp_send_key(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..c17769f 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_send_key(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/monitor.c b/monitor.c
> index 6fa0104..994b85c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1321,247 +1321,58 @@ static void do_sum(Monitor *mon, const QDict *qdict)
>      monitor_printf(mon, "%05d\n", sum);
>  }
>  
> -typedef struct {
> -    int keycode;
> -    const char *name;
> -} KeyDef;
> -
> -static const KeyDef key_defs[] = {
> -    { 0x2a, "shift" },
> -    { 0x36, "shift_r" },
> -
> -    { 0x38, "alt" },
> -    { 0xb8, "alt_r" },
> -    { 0x64, "altgr" },
> -    { 0xe4, "altgr_r" },
> -    { 0x1d, "ctrl" },
> -    { 0x9d, "ctrl_r" },
> -
> -    { 0xdd, "menu" },
> -
> -    { 0x01, "esc" },
> -
> -    { 0x02, "1" },
> -    { 0x03, "2" },
> -    { 0x04, "3" },
> -    { 0x05, "4" },
> -    { 0x06, "5" },
> -    { 0x07, "6" },
> -    { 0x08, "7" },
> -    { 0x09, "8" },
> -    { 0x0a, "9" },
> -    { 0x0b, "0" },
> -    { 0x0c, "minus" },
> -    { 0x0d, "equal" },
> -    { 0x0e, "backspace" },
> -
> -    { 0x0f, "tab" },
> -    { 0x10, "q" },
> -    { 0x11, "w" },
> -    { 0x12, "e" },
> -    { 0x13, "r" },
> -    { 0x14, "t" },
> -    { 0x15, "y" },
> -    { 0x16, "u" },
> -    { 0x17, "i" },
> -    { 0x18, "o" },
> -    { 0x19, "p" },
> -    { 0x1a, "bracket_left" },
> -    { 0x1b, "bracket_right" },
> -    { 0x1c, "ret" },
> -
> -    { 0x1e, "a" },
> -    { 0x1f, "s" },
> -    { 0x20, "d" },
> -    { 0x21, "f" },
> -    { 0x22, "g" },
> -    { 0x23, "h" },
> -    { 0x24, "j" },
> -    { 0x25, "k" },
> -    { 0x26, "l" },
> -    { 0x27, "semicolon" },
> -    { 0x28, "apostrophe" },
> -    { 0x29, "grave_accent" },
> -
> -    { 0x2b, "backslash" },
> -    { 0x2c, "z" },
> -    { 0x2d, "x" },
> -    { 0x2e, "c" },
> -    { 0x2f, "v" },
> -    { 0x30, "b" },
> -    { 0x31, "n" },
> -    { 0x32, "m" },
> -    { 0x33, "comma" },
> -    { 0x34, "dot" },
> -    { 0x35, "slash" },
> -
> -    { 0x37, "asterisk" },
> -
> -    { 0x39, "spc" },
> -    { 0x3a, "caps_lock" },
> -    { 0x3b, "f1" },
> -    { 0x3c, "f2" },
> -    { 0x3d, "f3" },
> -    { 0x3e, "f4" },
> -    { 0x3f, "f5" },
> -    { 0x40, "f6" },
> -    { 0x41, "f7" },
> -    { 0x42, "f8" },
> -    { 0x43, "f9" },
> -    { 0x44, "f10" },
> -    { 0x45, "num_lock" },
> -    { 0x46, "scroll_lock" },
> -
> -    { 0xb5, "kp_divide" },
> -    { 0x37, "kp_multiply" },
> -    { 0x4a, "kp_subtract" },
> -    { 0x4e, "kp_add" },
> -    { 0x9c, "kp_enter" },
> -    { 0x53, "kp_decimal" },
> -    { 0x54, "sysrq" },
> -
> -    { 0x52, "kp_0" },
> -    { 0x4f, "kp_1" },
> -    { 0x50, "kp_2" },
> -    { 0x51, "kp_3" },
> -    { 0x4b, "kp_4" },
> -    { 0x4c, "kp_5" },
> -    { 0x4d, "kp_6" },
> -    { 0x47, "kp_7" },
> -    { 0x48, "kp_8" },
> -    { 0x49, "kp_9" },
> -
> -    { 0x56, "less" },
> -
> -    { 0x57, "f11" },
> -    { 0x58, "f12" },
> -
> -    { 0xb7, "print" },
> -
> -    { 0xc7, "home" },
> -    { 0xc9, "pgup" },
> -    { 0xd1, "pgdn" },
> -    { 0xcf, "end" },
> -
> -    { 0xcb, "left" },
> -    { 0xc8, "up" },
> -    { 0xd0, "down" },
> -    { 0xcd, "right" },
> -
> -    { 0xd2, "insert" },
> -    { 0xd3, "delete" },
> -#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64)
> -    { 0xf0, "stop" },
> -    { 0xf1, "again" },
> -    { 0xf2, "props" },
> -    { 0xf3, "undo" },
> -    { 0xf4, "front" },
> -    { 0xf5, "copy" },
> -    { 0xf6, "open" },
> -    { 0xf7, "paste" },
> -    { 0xf8, "find" },
> -    { 0xf9, "cut" },
> -    { 0xfa, "lf" },
> -    { 0xfb, "help" },
> -    { 0xfc, "meta_l" },
> -    { 0xfd, "meta_r" },
> -    { 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.

> +            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.

>  
> -    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);
> +            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[p->value].keycode;
>          if (keycode & 0x80)
>              kbd_put_keycode(0xe0);
>          kbd_put_keycode(keycode & 0x7f);
>      }
> +
>      /* 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/qapi-schema.json b/qapi-schema.json
> index 3b6e346..08e51c6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1862,3 +1862,49 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> +
> +##
> +# @KeyCodes:

s/KeyCodes/KeyCode

> +#
> +# An enumeration of key name.
> +#
> +# This is used by the send-key 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' ] }
> +
> +##
> +# @send-key:
> +#
> +# Send keys to guest.
> +#
> +# @keys: key sequence. 'keys' could be the name of the key. Use a JSON array to
> +#        press several keys simultaneously.

s/could be/is

> +#
> +# @hold-time: #optional time to delay key up events, milliseconds. Defaults
> +#             to 200

The default is 100.

> +#
> +# Returns: Nothing on success
> +#          If key is unknown or redundant, InvalidParameter
> +#
> +# Since: 1.2
> +#
> +##
> +{ 'command': 'send-key',
> +  'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2e1a38e..12f6b76 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -335,6 +335,34 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "send-key",
> +        .args_type  = "keys:O,hold-time:i?",
> +        .mhandler.cmd_new = qmp_marshal_input_send_key,
> +    },
> +
> +SQMP
> +send-key
> +----------
> +
> +Send keys to VM.
> +
> +Arguments:
> +
> +keys array:
> +    - "key": key sequence (a json-array of key enum values)
> +
> +- hold-time: time to delay key up events, milliseconds. Defaults to 200
> +             (json-int, optional)
> +
> +Example:
> +
> +-> { "execute": "send-key",
> +     "arguments": { 'keys': [ 'ctrl', 'alt', 'delete' ] } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "cpu",
>          .args_type  = "index:i",
>          .mhandler.cmd_new = qmp_marshal_input_cpu,
Amos Kong July 18, 2012, 12:56 p.m. UTC | #2
On 12/07/12 23:09, Luiz Capitulino wrote:
>

Hi Luiz,

> On Thu,  5 Jul 2012 20:48:44 +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 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 <akong@redhat.com>
>> ---
>>   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.
Luiz Capitulino July 18, 2012, 1:50 p.m. UTC | #3
On Wed, 18 Jul 2012 20:56:54 +0800
Amos Kong <akong@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.

> 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.

> 
> 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().

> 
> 
> >> +            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.
>
Amos Kong July 18, 2012, 6:25 p.m. UTC | #4
----- Original Message -----
> On Wed, 18 Jul 2012 20:56:54 +0800
> Amos Kong <akong@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.
> > 
> 
> 
>
Amos Kong July 25, 2012, 5:55 a.m. UTC | #5
----- Original Message -----
> On Thu,  5 Jul 2012 20:48:44 +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 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 <akong@redhat.com>
> > ---
> >  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/qapi-schema.json b/qapi-schema.json
> > index 3b6e346..08e51c6 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1862,3 +1862,49 @@
> >  # Since: 0.14.0
> >  ##
> >  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> > +
> > +##
> > +# @KeyCodes:
> 
> s/KeyCodes/KeyCode


'KeyCode' is not an available variable name.

| ./qapi-types.h:471: error: conflicting types for ‘KeyCode’
| /usr/include/X11/X.h:108: note: previous declaration of ‘KeyCode’ was here

How about 'CodeOfKey'?

> > +#
> > +# An enumeration of key name.
> > +#
> > +# This is used by the send-key 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' ] }
Luiz Capitulino July 25, 2012, 12:38 p.m. UTC | #6
On Wed, 25 Jul 2012 01:55:14 -0400 (EDT)
Amos Kong <akong@redhat.com> wrote:

> 
> 
> ----- Original Message -----
> > On Thu,  5 Jul 2012 20:48:44 +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 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 <akong@redhat.com>
> > > ---
> > >  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/qapi-schema.json b/qapi-schema.json
> > > index 3b6e346..08e51c6 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -1862,3 +1862,49 @@
> > >  # Since: 0.14.0
> > >  ##
> > >  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> > > +
> > > +##
> > > +# @KeyCodes:
> > 
> > s/KeyCodes/KeyCode
> 
> 
> 'KeyCode' is not an available variable name.
> 
> | ./qapi-types.h:471: error: conflicting types for ‘KeyCode’
> | /usr/include/X11/X.h:108: note: previous declaration of ‘KeyCode’ was here
> 
> How about 'CodeOfKey'?

QKeyCode, maybe?

Can you please paste the full error message?

> 
> > > +#
> > > +# An enumeration of key name.
> > > +#
> > > +# This is used by the send-key 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' ] }
>
Amos Kong July 25, 2012, 1:56 p.m. UTC | #7
----- Original Message -----
> On Wed, 25 Jul 2012 01:55:14 -0400 (EDT)
> Amos Kong <akong@redhat.com> wrote:
> 
> > 
> > 
> > ----- Original Message -----
> > > On Thu,  5 Jul 2012 20:48:44 +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 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 <akong@redhat.com>
> > > > ---
> > > >  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/qapi-schema.json b/qapi-schema.json
> > > > index 3b6e346..08e51c6 100644
> > > > --- a/qapi-schema.json
> > > > +++ b/qapi-schema.json
> > > > @@ -1862,3 +1862,49 @@
> > > >  # Since: 0.14.0
> > > >  ##
> > > >  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> > > > +
> > > > +##
> > > > +# @KeyCodes:
> > > 
> > > s/KeyCodes/KeyCode
> > 
> > 
> > 'KeyCode' is not an available variable name.
> > 
> > | ./qapi-types.h:471: error: conflicting types for ‘KeyCode’
> > | /usr/include/X11/X.h:108: note: previous declaration of ‘KeyCode’
> > | was here
> > 
> > How about 'CodeOfKey'?
> 
> QKeyCode, maybe?

Looks good.

> Can you please paste the full error message?

[root@dhcp-8-167 qemu]# make 
  ....
  CC    slirp/arp_table.o
  CC    ui/keymaps.o
  CC    ui/spice-core.o
  CC    ui/spice-input.o
  CC    ui/spice-display.o
  CC    ui/sdl.o
In file included from ./console.h:9,
                 from ui/sdl.c:32:
./qapi-types.h:471: error: conflicting types for ‘KeyCode’
/usr/include/X11/X.h:108: note: previous declaration of ‘KeyCode’ was here
make: *** [ui/sdl.o] Error 1
[root@dhcp-8-167 qemu]#
diff mbox

Patch

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;
+} KeyDef;
+
+static const KeyDef key_defs[] = {
+    [KEY_CODES_SHIFT] = { 0x2a, "shift" },
+    [KEY_CODES_SHIFT_R] = { 0x36, "shift_r" },
+
+    [KEY_CODES_ALT] = { 0x38, "alt" },
+    [KEY_CODES_ALT_R] = { 0xb8, "alt_r" },
+    [KEY_CODES_ALTGR] = { 0x64, "altgr" },
+    [KEY_CODES_ALTGR_R] = { 0xe4, "altgr_r" },
+    [KEY_CODES_CTRL] = { 0x1d, "ctrl" },
+    [KEY_CODES_CTRL_R] = { 0x9d, "ctrl_r" },
+
+    [KEY_CODES_MENU] = { 0xdd, "menu" },
+
+    [KEY_CODES_ESC] = { 0x01, "esc" },
+
+    [KEY_CODES_1] = { 0x02, "1" },
+    [KEY_CODES_2] = { 0x03, "2" },
+    [KEY_CODES_3] = { 0x04, "3" },
+    [KEY_CODES_4] = { 0x05, "4" },
+    [KEY_CODES_5] = { 0x06, "5" },
+    [KEY_CODES_6] = { 0x07, "6" },
+    [KEY_CODES_7] = { 0x08, "7" },
+    [KEY_CODES_8] = { 0x09, "8" },
+    [KEY_CODES_9] = { 0x0a, "9" },
+    [KEY_CODES_0] = { 0x0b, "0" },
+    [KEY_CODES_MINUS] = { 0x0c, "minus" },
+    [KEY_CODES_EQUAL] = { 0x0d, "equal" },
+    [KEY_CODES_BACKSPACE] = { 0x0e, "backspace" },
+
+    [KEY_CODES_TAB] = { 0x0f, "tab" },
+    [KEY_CODES_Q] = { 0x10, "q" },
+    [KEY_CODES_W] = { 0x11, "w" },
+    [KEY_CODES_E] = { 0x12, "e" },
+    [KEY_CODES_R] = { 0x13, "r" },
+    [KEY_CODES_T] = { 0x14, "t" },
+    [KEY_CODES_Y] = { 0x15, "y" },
+    [KEY_CODES_U] = { 0x16, "u" },
+    [KEY_CODES_I] = { 0x17, "i" },
+    [KEY_CODES_O] = { 0x18, "o" },
+    [KEY_CODES_P] = { 0x19, "p" },
+    [KEY_CODES_BRACKET_LEFT] = { 0x1a, "bracket_left" },
+    [KEY_CODES_BRACKET_RIGHT] = { 0x1b, "bracket_right" },
+    [KEY_CODES_RET] = { 0x1c, "ret" },
+
+    [KEY_CODES_A] = { 0x1e, "a" },
+    [KEY_CODES_S] = { 0x1f, "s" },
+    [KEY_CODES_D] = { 0x20, "d" },
+    [KEY_CODES_F] = { 0x21, "f" },
+    [KEY_CODES_G] = { 0x22, "g" },
+    [KEY_CODES_H] = { 0x23, "h" },
+    [KEY_CODES_J] = { 0x24, "j" },
+    [KEY_CODES_K] = { 0x25, "k" },
+    [KEY_CODES_L] = { 0x26, "l" },
+    [KEY_CODES_SEMICOLON] = { 0x27, "semicolon" },
+    [KEY_CODES_APOSTROPHE] = { 0x28, "apostrophe" },
+    [KEY_CODES_GRAVE_ACCENT] = { 0x29, "grave_accent" },
+
+    [KEY_CODES_BACKSLASH] = { 0x2b, "backslash" },
+    [KEY_CODES_Z] = { 0x2c, "z" },
+    [KEY_CODES_X] = { 0x2d, "x" },
+    [KEY_CODES_C] = { 0x2e, "c" },
+    [KEY_CODES_V] = { 0x2f, "v" },
+    [KEY_CODES_B] = { 0x30, "b" },
+    [KEY_CODES_N] = { 0x31, "n" },
+    [KEY_CODES_M] = { 0x32, "m" },
+    [KEY_CODES_COMMA] = { 0x33, "comma" },
+    [KEY_CODES_DOT] = { 0x34, "dot" },
+    [KEY_CODES_SLASH] = { 0x35, "slash" },
+
+    [KEY_CODES_ASTERISK] = { 0x37, "asterisk" },
+
+    [KEY_CODES_SPC] = { 0x39, "spc" },
+    [KEY_CODES_CAPS_LOCK] = { 0x3a, "caps_lock" },
+    [KEY_CODES_F1] = { 0x3b, "f1" },
+    [KEY_CODES_F2] = { 0x3c, "f2" },
+    [KEY_CODES_F3] = { 0x3d, "f3" },
+    [KEY_CODES_F4] = { 0x3e, "f4" },
+    [KEY_CODES_F5] = { 0x3f, "f5" },
+    [KEY_CODES_F6] = { 0x40, "f6" },
+    [KEY_CODES_F7] = { 0x41, "f7" },
+    [KEY_CODES_F8] = { 0x42, "f8" },
+    [KEY_CODES_F9] = { 0x43, "f9" },
+    [KEY_CODES_F10] = { 0x44, "f10" },
+    [KEY_CODES_NUM_LOCK] = { 0x45, "num_lock" },
+    [KEY_CODES_SCROLL_LOCK] = { 0x46, "scroll_lock" },
+
+    [KEY_CODES_KP_DIVIDE] = { 0xb5, "kp_divide" },
+    [KEY_CODES_KP_MULTIPLY] = { 0x37, "kp_multiply" },
+    [KEY_CODES_KP_SUBTRACT] = { 0x4a, "kp_subtract" },
+    [KEY_CODES_KP_ADD] = { 0x4e, "kp_add" },
+    [KEY_CODES_KP_ENTER] = { 0x9c, "kp_enter" },
+    [KEY_CODES_KP_DECIMAL] = { 0x53, "kp_decimal" },
+    [KEY_CODES_SYSRQ] = { 0x54, "sysrq" },
+
+    [KEY_CODES_KP_0] = { 0x52, "kp_0" },
+    [KEY_CODES_KP_1] = { 0x4f, "kp_1" },
+    [KEY_CODES_KP_2] = { 0x50, "kp_2" },
+    [KEY_CODES_KP_3] = { 0x51, "kp_3" },
+    [KEY_CODES_KP_4] = { 0x4b, "kp_4" },
+    [KEY_CODES_KP_5] = { 0x4c, "kp_5" },
+    [KEY_CODES_KP_6] = { 0x4d, "kp_6" },
+    [KEY_CODES_KP_7] = { 0x47, "kp_7" },
+    [KEY_CODES_KP_8] = { 0x48, "kp_8" },
+    [KEY_CODES_KP_9] = { 0x49, "kp_9" },
+
+    [KEY_CODES_LESS] = { 0x56, "less" },
+
+    [KEY_CODES_F11] = { 0x57, "f11" },
+    [KEY_CODES_F12] = { 0x58, "f12" },
+
+    [KEY_CODES_PRINT] = { 0xb7, "print" },
+
+    [KEY_CODES_HOME] = { 0xc7, "home" },
+    [KEY_CODES_PGUP] = { 0xc9, "pgup" },
+    [KEY_CODES_PGDN] = { 0xd1, "pgdn" },
+    [KEY_CODES_END] = { 0xcf, "end" },
+
+    [KEY_CODES_LEFT] = { 0xcb, "left" },
+    [KEY_CODES_UP] = { 0xc8, "up" },
+    [KEY_CODES_DOWN] = { 0xd0, "down" },
+    [KEY_CODES_RIGHT] = { 0xcd, "right" },
+
+    [KEY_CODES_INSERT] = { 0xd2, "insert" },
+    [KEY_CODES_DELETE] = { 0xd3, "delete" },
+#ifdef NEED_CPU_H
+#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64)
+    [KEY_CODES_STOP] = { 0xf0, "stop" },
+    [KEY_CODES_AGAIN] = { 0xf1, "again" },
+    [KEY_CODES_PROPS] = { 0xf2, "props" },
+    [KEY_CODES_UNDO] = { 0xf3, "undo" },
+    [KEY_CODES_FRONT] = { 0xf4, "front" },
+    [KEY_CODES_COPY] = { 0xf5, "copy" },
+    [KEY_CODES_OPEN] = { 0xf6, "open" },
+    [KEY_CODES_PASTE] = { 0xf7, "paste" },
+    [KEY_CODES_FIND] = { 0xf8, "find" },
+    [KEY_CODES_CUT] = { 0xf9, "cut" },
+    [KEY_CODES_LF] = { 0xfa, "lf" },
+    [KEY_CODES_HELP] = { 0xfb, "help" },
+    [KEY_CODES_META_L] = { 0xfc, "meta_l" },
+    [KEY_CODES_META_R] = { 0xfd, "meta_r" },
+    [KEY_CODES_COMPOSE] = { 0xfe, "compose" },
+#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;
+
+    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;
+    }
+
+    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);
+        keylist->next = NULL;
+
+        if (tmp)
+            tmp->next = keylist;
+        tmp = keylist;
+        if (!head)
+            head = keylist;
+
+        if (!separator)
+            break;
+        keys = separator + 1;
+    }
+
+    qmp_send_key(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..c17769f 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_send_key(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index 6fa0104..994b85c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1321,247 +1321,58 @@  static void do_sum(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "%05d\n", sum);
 }
 
-typedef struct {
-    int keycode;
-    const char *name;
-} KeyDef;
-
-static const KeyDef key_defs[] = {
-    { 0x2a, "shift" },
-    { 0x36, "shift_r" },
-
-    { 0x38, "alt" },
-    { 0xb8, "alt_r" },
-    { 0x64, "altgr" },
-    { 0xe4, "altgr_r" },
-    { 0x1d, "ctrl" },
-    { 0x9d, "ctrl_r" },
-
-    { 0xdd, "menu" },
-
-    { 0x01, "esc" },
-
-    { 0x02, "1" },
-    { 0x03, "2" },
-    { 0x04, "3" },
-    { 0x05, "4" },
-    { 0x06, "5" },
-    { 0x07, "6" },
-    { 0x08, "7" },
-    { 0x09, "8" },
-    { 0x0a, "9" },
-    { 0x0b, "0" },
-    { 0x0c, "minus" },
-    { 0x0d, "equal" },
-    { 0x0e, "backspace" },
-
-    { 0x0f, "tab" },
-    { 0x10, "q" },
-    { 0x11, "w" },
-    { 0x12, "e" },
-    { 0x13, "r" },
-    { 0x14, "t" },
-    { 0x15, "y" },
-    { 0x16, "u" },
-    { 0x17, "i" },
-    { 0x18, "o" },
-    { 0x19, "p" },
-    { 0x1a, "bracket_left" },
-    { 0x1b, "bracket_right" },
-    { 0x1c, "ret" },
-
-    { 0x1e, "a" },
-    { 0x1f, "s" },
-    { 0x20, "d" },
-    { 0x21, "f" },
-    { 0x22, "g" },
-    { 0x23, "h" },
-    { 0x24, "j" },
-    { 0x25, "k" },
-    { 0x26, "l" },
-    { 0x27, "semicolon" },
-    { 0x28, "apostrophe" },
-    { 0x29, "grave_accent" },
-
-    { 0x2b, "backslash" },
-    { 0x2c, "z" },
-    { 0x2d, "x" },
-    { 0x2e, "c" },
-    { 0x2f, "v" },
-    { 0x30, "b" },
-    { 0x31, "n" },
-    { 0x32, "m" },
-    { 0x33, "comma" },
-    { 0x34, "dot" },
-    { 0x35, "slash" },
-
-    { 0x37, "asterisk" },
-
-    { 0x39, "spc" },
-    { 0x3a, "caps_lock" },
-    { 0x3b, "f1" },
-    { 0x3c, "f2" },
-    { 0x3d, "f3" },
-    { 0x3e, "f4" },
-    { 0x3f, "f5" },
-    { 0x40, "f6" },
-    { 0x41, "f7" },
-    { 0x42, "f8" },
-    { 0x43, "f9" },
-    { 0x44, "f10" },
-    { 0x45, "num_lock" },
-    { 0x46, "scroll_lock" },
-
-    { 0xb5, "kp_divide" },
-    { 0x37, "kp_multiply" },
-    { 0x4a, "kp_subtract" },
-    { 0x4e, "kp_add" },
-    { 0x9c, "kp_enter" },
-    { 0x53, "kp_decimal" },
-    { 0x54, "sysrq" },
-
-    { 0x52, "kp_0" },
-    { 0x4f, "kp_1" },
-    { 0x50, "kp_2" },
-    { 0x51, "kp_3" },
-    { 0x4b, "kp_4" },
-    { 0x4c, "kp_5" },
-    { 0x4d, "kp_6" },
-    { 0x47, "kp_7" },
-    { 0x48, "kp_8" },
-    { 0x49, "kp_9" },
-
-    { 0x56, "less" },
-
-    { 0x57, "f11" },
-    { 0x58, "f12" },
-
-    { 0xb7, "print" },
-
-    { 0xc7, "home" },
-    { 0xc9, "pgup" },
-    { 0xd1, "pgdn" },
-    { 0xcf, "end" },
-
-    { 0xcb, "left" },
-    { 0xc8, "up" },
-    { 0xd0, "down" },
-    { 0xcd, "right" },
-
-    { 0xd2, "insert" },
-    { 0xd3, "delete" },
-#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64)
-    { 0xf0, "stop" },
-    { 0xf1, "again" },
-    { 0xf2, "props" },
-    { 0xf3, "undo" },
-    { 0xf4, "front" },
-    { 0xf5, "copy" },
-    { 0xf6, "open" },
-    { 0xf7, "paste" },
-    { 0xf8, "find" },
-    { 0xf9, "cut" },
-    { 0xfa, "lf" },
-    { 0xfb, "help" },
-    { 0xfc, "meta_l" },
-    { 0xfd, "meta_r" },
-    { 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) {
+            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);
     }
 }
 
-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;
 
-    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;
-            }
+    keycodes = keys;
 
-            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) {
+            sprintf(value, "%d", p->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[p->value].keycode;
         if (keycode & 0x80)
             kbd_put_keycode(0xe0);
         kbd_put_keycode(keycode & 0x7f);
     }
+
     /* 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/qapi-schema.json b/qapi-schema.json
index 3b6e346..08e51c6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1862,3 +1862,49 @@ 
 # Since: 0.14.0
 ##
 { 'command': 'netdev_del', 'data': {'id': 'str'} }
+
+##
+# @KeyCodes:
+#
+# An enumeration of key name.
+#
+# This is used by the send-key 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' ] }
+
+##
+# @send-key:
+#
+# Send keys to guest.
+#
+# @keys: key sequence. 'keys' could be the name of the key. Use a JSON array to
+#        press several keys simultaneously.
+#
+# @hold-time: #optional time to delay key up events, milliseconds. Defaults
+#             to 200
+#
+# Returns: Nothing on success
+#          If key is unknown or redundant, InvalidParameter
+#
+# Since: 1.2
+#
+##
+{ 'command': 'send-key',
+  'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2e1a38e..12f6b76 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -335,6 +335,34 @@  Example:
 EQMP
 
     {
+        .name       = "send-key",
+        .args_type  = "keys:O,hold-time:i?",
+        .mhandler.cmd_new = qmp_marshal_input_send_key,
+    },
+
+SQMP
+send-key
+----------
+
+Send keys to VM.
+
+Arguments:
+
+keys array:
+    - "key": key sequence (a json-array of key enum values)
+
+- hold-time: time to delay key up events, milliseconds. Defaults to 200
+             (json-int, optional)
+
+Example:
+
+-> { "execute": "send-key",
+     "arguments": { 'keys': [ 'ctrl', 'alt', 'delete' ] } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "cpu",
         .args_type  = "index:i",
         .mhandler.cmd_new = qmp_marshal_input_cpu,