Re: [Qemu-devel] [PATCH v3 5/6] qapi: convert sendkey

[Amos Kong]

On 27/06/12 04:21, Luiz Capitulino wrote:
> On Wed, 20 Jun 2012 12:47:40 +0800
> Amos Kong<address@hidden>  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<address@hidden>
> > ---
> >   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.

--
                        Amos.