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

[Amos Kong]

On 12/07/12 23:09, Luiz Capitulino wrote:
>

Hi Luiz,

> On Thu,  5 Jul 2012 20:48:44 +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 console.h is the mapping of key name to keycode,
> > Keys' index in the enmu and key_defs[] is same.
> >
> > 'send-key' of QMP doesn't support key in hexadecimal format.
> >
> > Signed-off-by: Amos Kong <address@hidden>
> > ---
> >   console.h        |  152 ++++++++++++++++++++++++++++++++++
> >   hmp-commands.hx  |    2 +-
> >   hmp.c            |   64 +++++++++++++++
> >   hmp.h            |    1 +
> >   monitor.c        |  239 ++++++------------------------------------------------
> >   qapi-schema.json |   46 +++++++++++
> >   qmp-commands.hx  |   28 +++++++
> >   7 files changed, 317 insertions(+), 215 deletions(-)
> >
> > diff --git a/console.h b/console.h
> > index 4334db5..e1b0c45 100644
> > --- a/console.h
> > +++ b/console.h
> > @@ -6,6 +6,7 @@
> >   #include "notify.h"
> >   #include "monitor.h"
> >   #include "trace.h"
> > +#include "qapi-types.h"
> >
> >   /* keyboard/mouse support */
> >
> > @@ -397,4 +398,155 @@ static inline int vnc_display_pw_expire(DisplayState *ds, 
> > time_t expires)
> >   /* curses.c */
> >   void curses_display_init(DisplayState *ds, int full_screen);
> >
> > +typedef struct {
> > +    int keycode;
> > +    const char *name;
>
> I don't think we need 'name', as key names are also provided by 
> KeyCodes_lookup[].
> See more on this below.


Yes, I tried to define key_defs[] to a int array, and get keyname from 
KeyCodes_loopup[],
it works.

const int key_defs[] = {
    [KEY_CODES_SHIFT] = 0x2a,
    ....


> > +} KeyDef;
> > +
> > +static const KeyDef key_defs[] = {
>
> We can't have an array defined in a header file because it will be defined in
> each .c file that includes it.
>
> Please, define it in input.c (along with qmp_send_key())

Ok.

> and write the following public functions:
>
>   o KeyCode keycode_from_key(const char *key);
>   o KeyCode keycode_from_code(int code);


void qmp_send_key(KeyCodesList *keys, bool has_hold_time, int64_t 
hold_time, ...)
                    ^
                    \_ when we use qmp, a key list will be passed, the 
values are the index
                       in enum KeyCodes. not the real KeyCode.

                    { 'enum': 'KeyCodes',
                      'data': [ 'shift', 'shift_r', 'al...

So we need to get this kind of 'index' in hmp_send_key() and pass to 
qmp_send_key().
then convert this 'index' to keycode in qmp_send_key()

I didn't find a way to define a non-serial enum.

eg: (then int qmp_marshal_input_send_key() would pass real keycode to 
qmp_send_key())
{ 'enum': 'KeyCodes',
  'data': [ 'shift' = 0x2a, 'shift_r' = 0x36, 'alt' = 0x38, ...


If we still pass 'index' to qmp_send_key as patch V4.

extern int index_from_key(const char *key);   -> it's used in hmp_send_key()
extern int index_from_keycode(int code);      -> it's used in hmp_send_key()
extern char *key_from_keycode(int idx);       -> it's used in 
monitor_find_completion()
extern int keycode_from_key(const char *key); -> it's used in qmp_send_key()


> and then use these functions where using key_defs would be necessary. Also,
> note that keycode_from_key() can use KeyCodes_lookup[] instead of key_defs (this
> way we can drop 'name' from KeyDef).

....

> > +#endif
> > +#endif
> > +    [KEY_CODES_MAX] = { 0, NULL },
> > +};
> > +
> >   #endif
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index e336251..865eea9 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -505,7 +505,7 @@ ETEXI
> >           .args_type  = "keys:s,hold-time:i?",
> >           .params     = "keys [hold_ms]",
> >           .help       = "send keys to the VM (e.g. 'sendkey ctrl-alt-f1', default 
> > hold time=100 ms)",
> > -        .mhandler.cmd = do_sendkey,
> > +        .mhandler.cmd = hmp_send_key,
> >       },
> >
> >   STEXI
> > diff --git a/hmp.c b/hmp.c
> > index b9cec1d..cfdc106 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -19,6 +19,7 @@
> >   #include "qemu-timer.h"
> >   #include "qmp-commands.h"
> >   #include "monitor.h"
> > +#include "console.h"
> >
> >   static void hmp_handle_error(Monitor *mon, Error **errp)
> >   {
> > @@ -1000,3 +1001,66 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> >       qmp_netdev_del(id,&err);
> >       hmp_handle_error(mon,&err);
> >   }
> > +
> > +static int get_key_index(const char *key)
> > +{
> > +    int i, keycode;
> > +    char *endp;
> > +
> > +    for (i = 0; i<  KEY_CODES_MAX; i++)
> > +        if (key_defs[i].keycode&&  !strcmp(key, key_defs[i].name))
> > +            return i;
>
> Here you can call do:
>
>    keycode = keycode_from_key(key);
>    if (keycode != KEY_CODES_MAX) {
>         return keycode;
>    }
>
> > +
> > +    if (strstart(key, "0x", NULL)) {
> > +        keycode = strtoul(key,&endp, 0);
> > +        if (*endp == '\0'&&  keycode>= 0x01&&  keycode<= 0xff)
> > +            for (i = 0; i<  KEY_CODES_MAX; i++)
> > +                if (keycode == key_defs[i].keycode)
> > +                    return i;
>
> You can drop that for loop and do instead:
>
>    keycode = keycode_from_code(keycode);
>
>
> > +    }
> > +
> > +    return -1;
> > +}
> > +
> > +void hmp_send_key(Monitor *mon, const QDict *qdict)
> > +{
> > +    const char *keys = qdict_get_str(qdict, "keys");
> > +    KeyCodesList *keylist, *head = NULL, *tmp = NULL;
> > +    int has_hold_time = qdict_haskey(qdict, "hold-time");
> > +    int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
> > +    Error *err = NULL;
> > +    char keyname_buf[16];
> > +    char *separator;
> > +    int keyname_len;
> > +
> > +    while (1) {
> > +        separator = strchr(keys, '-');
> > +        keyname_len = separator ? separator - keys : strlen(keys);
> > +        pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
> > +
> > +        /* Be compatible with old interface, convert user inputted "<" */
> > +        if (!strncmp(keyname_buf, "<", 1)&&  keyname_len == 1) {
> > +            pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
> > +            keyname_len = 4;
> > +        }
> > +        keyname_buf[keyname_len] = 0;
> > +
> > +        keylist = g_malloc0(sizeof(*keylist));
> > +        keylist->value = get_key_index(keyname_buf);
>
> get_key_index() can fail.

Ok, I would check it.

> > +        keylist->next = NULL;
> > +
> > +        if (tmp)
> > +            tmp->next = keylist;
> > +        tmp = keylist;
> > +        if (!head)
> > +            head = keylist;
> > +
> > +        if (!separator)
> > +            break;
> > +        keys = separator + 1;
> > +    }

...

> > -    { 0xfe, "compose" },
> > -#endif
> > -    { 0, NULL },
> > -};
> > -
> > -static int get_keycode(const char *key)
> > -{
> > -    const KeyDef *p;
> > -    char *endp;
> > -    int ret;
> >
> > -    for(p = key_defs; p->name != NULL; p++) {
> > -        if (!strcmp(key, p->name))
> > -            return p->keycode;
> > -    }
> > -    if (strstart(key, "0x", NULL)) {
> > -        ret = strtoul(key,&endp, 0);
> > -        if (*endp == '\0'&&  ret>= 0x01&&  ret<= 0xff)
> > -            return ret;
> > -    }
> > -    return -1;
> > -}
> > -
> > -#define MAX_KEYCODES 16
> > -static uint8_t keycodes[MAX_KEYCODES];
> > -static int nb_pending_keycodes;
> > +static KeyCodesList *keycodes;
> >   static QEMUTimer *key_timer;
> >
> >   static void release_keys(void *opaque)
> >   {
> >       int keycode;
> > +    KeyCodesList *p;
> > +
> > +    for (p = keycodes; p != NULL; p = p->next) {
> > +        if (p->value>  KEY_CODES_MAX) {
>
> This check is not needed, as far as I can understand only qmp_send_key() can put
> keys in the keycodes list and qmp_send_key() does this check already.

If we find one invalid 'value', other keys in the list will be ignored.
so we don't need to release them here.


> > +            keycodes = NULL;
> > +            break;
> > +        }
> >
> > -    while (nb_pending_keycodes>  0) {
> > -        nb_pending_keycodes--;
> > -        keycode = keycodes[nb_pending_keycodes];
> > +        keycode = key_defs[p->value].keycode;
> >           if (keycode&  0x80)
> >               kbd_put_keycode(0xe0);
> >           kbd_put_keycode(keycode | 0x80);
> >       }
>
> Please set keycodes to NULL here. Actually, you'll have to free it first,
> because keycodes has to be duplicated (see below).
>
> >   }
> >
> > -static void do_sendkey(Monitor *mon, const QDict *qdict)
> > +void qmp_send_key(KeyCodesList *keys, bool has_hold_time, int64_t hold_time,
> > +                 Error **errp)
> >   {
> > -    char keyname_buf[16];
> > -    char *separator;
> > -    int keyname_len, keycode, i;
> > -    const char *keys = qdict_get_str(qdict, "keys");
> > -    int has_hold_time = qdict_haskey(qdict, "hold-time");
> > -    int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
> > +    int keycode;
> > +    char value[5];
> > +    KeyCodesList *p;
>
> Let's initialize key_timer here, like this:
>
> if (!key_timer) {
>      key_timer = qemu_new_timer_ns(vm_clock, release_keys, NULL);
> }
>
> Then drop the initialization done in monitor_init(). This way we untangle
> qmp_send_key() from the monitor.
>
> Also, please, move qmp_send_key(), release_keys, etc to input.c as I said
> above.

Ok.

> > -    if (nb_pending_keycodes>  0) {
> > +    if (keycodes != NULL) {
> >           qemu_del_timer(key_timer);
> >           release_keys(NULL);
> >       }
> >       if (!has_hold_time)
> >           hold_time = 100;
>
> Please, add { } around the if body above.
>
> > -    i = 0;
> > -    while (1) {
> > -        separator = strchr(keys, '-');
> > -        keyname_len = separator ? separator - keys : strlen(keys);
> > -        if (keyname_len>  0) {
> > -            pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
> > -            if (keyname_len>  sizeof(keyname_buf) - 1) {
> > -                monitor_printf(mon, "invalid key: '%s...'\n", keyname_buf);
> > -                return;
> > -            }
> > -            if (i == MAX_KEYCODES) {
> > -                monitor_printf(mon, "too many keys\n");
> > -                return;
> > -            }
> >
> > -            /* Be compatible with old interface, convert user inputted "<" */
> > -            if (!strncmp(keyname_buf, "<", 1)&&  keyname_len == 1) {
> > -                pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
> > -                keyname_len = 4;
> > -            }
> > +    keycodes = keys;
>
> It's better to this assignment after the for loop below. Actually, you have to
> duplicate the key list because the qapi code will free it as soon as
> qmp_send_key() returns, but it will be used later in the timer handler.
>
> > -            keyname_buf[keyname_len] = 0;
> > -            keycode = get_keycode(keyname_buf);
> > -            if (keycode<  0) {
> > -                monitor_printf(mon, "unknown key: '%s'\n", keyname_buf);
> > -                return;
> > -            }
> > -            keycodes[i++] = keycode;
> > +    for (p = keycodes; p != NULL; p = p->next) {
> > +        if (p->value>  KEY_CODES_MAX) {
>
> You should test against>=.
>
> > +            sprintf(value, "%d", p->value);
> > +            error_set(errp, QERR_INVALID_PARAMETER, value);

^^ If an invalid key is found, the other keys will be ignored.


Will fix other issues you mentioned, thanks for your review.

--
                        Amos.