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

[Amos Kong <akong@redhat.com>]

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

Hi Luiz,

> On Thu,  5 Jul 2012 20:48:44 +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 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 <akong@redhat.com>
>> ---
>>   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.