Patchwork [v3,5/6] qapi: convert sendkey

login
register
mail settings
Submitter Amos Kong
Date June 20, 2012, 4:47 a.m.
Message ID <1340167661-25499-6-git-send-email-akong@redhat.com>
Download mbox | patch
Permalink /patch/165881/
State New
Headers show

Comments

Amos Kong - June 20, 2012, 4:47 a.m.
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(-)
Eric Blake - June 20, 2012, 12:53 p.m.
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/
Luiz Capitulino - June 26, 2012, 8:21 p.m.
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,
Luiz Capitulino - June 26, 2012, 8:22 p.m.
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/
>
Amos Kong - June 27, 2012, 9:59 a.m.
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.
Amos Kong - June 27, 2012, 10:22 a.m.
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.
Eric Blake - June 27, 2012, 12:22 p.m.
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.
Luiz Capitulino - June 27, 2012, 12:53 p.m.
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.
Luiz Capitulino - June 27, 2012, 1:19 p.m.
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.
>

Patch

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,