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

[Luiz Capitulino]

On Wed, 18 Jul 2012 20:56:54 +0800
Amos Kong <address@hidden> 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.
>