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

[Amos Kong]

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

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