[Qemu-devel] [PATCH 0/3]: qmp: send-key: accept key codes in hex

[Qemu-devel] [PATCH 0/3]: qmp: send-key: accept key codes in hex

[Luiz Capitulino]

This actually fixes a regression introduced by the qapi conversion,
please refer to patch 2/3 for details.

It's also important to note that this series changes the QMP
interface for the send-key command, but this shouldn't be a problem
as we're still in development phase.

 hmp.c            | 43 +++++++++++++++++++++++------------
 input.c          | 69 +++++++++++++++++++++++++++-----------------------------
 qapi-schema.json | 19 +++++++++++++---
 3 files changed, 78 insertions(+), 53 deletions(-)

[Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify

[Luiz Capitulino]

The current code duplicates the QKeyCodeList keys in order to store
the key values for release_keys() late run. This is a bit complicated
though, as we have to care about correct ordering and then release_keys()
will have to index key_defs[] over again.

Switch to an array of integers, which is dynamically allocated and stores
the already converted key value.

This simplies the current code and the next commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 input.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/input.c b/input.c
index c4b0619..32c6057 100644
--- a/input.c
+++ b/input.c
@@ -224,30 +224,31 @@ int index_from_keycode(int code)
     return i;
 }
 
-static QKeyCodeList *keycodes;
+static int *keycodes;
+static int keycodes_size;
 static QEMUTimer *key_timer;
 
 static void release_keys(void *opaque)
 {
-    int keycode;
-    QKeyCodeList *p;
+    int i;
 
-    for (p = keycodes; p != NULL; p = p->next) {
-        keycode = key_defs[p->value];
-        if (keycode & 0x80) {
+    for (i = 0; i < keycodes_size; i++) {
+        if (keycodes[i] & 0x80) {
             kbd_put_keycode(0xe0);
         }
-        kbd_put_keycode(keycode | 0x80);
+        kbd_put_keycode(keycodes[i]| 0x80);
     }
-    qapi_free_QKeyCodeList(keycodes);
+
+    g_free(keycodes);
     keycodes = NULL;
+    keycodes_size = 0;
 }
 
 void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
                   Error **errp)
 {
     int keycode;
-    QKeyCodeList *p, *keylist, *head = NULL, *tmp = NULL;
+    QKeyCodeList *p;
 
     if (!key_timer) {
         key_timer = qemu_new_timer_ns(vm_clock, release_keys, NULL);
@@ -257,31 +258,22 @@ void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
         qemu_del_timer(key_timer);
         release_keys(NULL);
     }
+
     if (!has_hold_time) {
         hold_time = 100;
     }
 
     for (p = keys; p != NULL; p = p->next) {
-        keylist = g_malloc0(sizeof(*keylist));
-        keylist->value = p->value;
-        keylist->next = NULL;
-
-        if (!head) {
-            head = keylist;
-        }
-        if (tmp) {
-            tmp->next = keylist;
-        }
-        tmp = keylist;
-
         /* key down events */
         keycode = key_defs[p->value];
         if (keycode & 0x80) {
             kbd_put_keycode(0xe0);
         }
         kbd_put_keycode(keycode & 0x7f);
+
+        keycodes = g_realloc(keycodes, sizeof(int) * (keycodes_size + 1));
+        keycodes[keycodes_size++] = keycode;
     }
-    keycodes = head;
 
     /* delayed key up events */
     qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) +
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex

[Luiz Capitulino]

Before the qapi conversion, the sendkey command could be used to
send key codes in hex directly to the guest. This doesn't work
with the current implemention, as it will only send to the guest
QKeyCode values. That's a regression.

This commit fixes the problem by adding hex value support down
the QMP interface, qmp_send_key().

In more detail, this commit:

 1. Adds the KeyValue union. This can represent an hex value or
    a QKeyCode value

 2. *Changes* the QMP send-key command to take an KeyValue argument
    instead of a QKeyCode one.

 3. Adapt hmp_send_key() to the QMP interface changes

Item 2 is an incompatible change, but as we're development phase
(and this command has been merged a few weeks ago) this shouldn't be
a problem.

Lastly, it's not possible to split this commit without breaking the
build.

Reported-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp.c            | 43 +++++++++++++++++++++++++++++--------------
 input.c          | 23 ++++++++++++++++++++---
 qapi-schema.json | 19 ++++++++++++++++---
 3 files changed, 65 insertions(+), 20 deletions(-)

diff --git a/hmp.c b/hmp.c
index ba6fbd3..3e64913 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1109,13 +1109,13 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
 void hmp_send_key(Monitor *mon, const QDict *qdict)
 {
     const char *keys = qdict_get_str(qdict, "keys");
-    QKeyCodeList *keylist, *head = NULL, *tmp = NULL;
+    KeyValueList *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, idx;
+    int keyname_len;
 
     while (1) {
         separator = strchr(keys, '-');
@@ -1129,15 +1129,8 @@ void hmp_send_key(Monitor *mon, const QDict *qdict)
         }
         keyname_buf[keyname_len] = 0;
 
-        idx = index_from_key(keyname_buf);
-        if (idx == Q_KEY_CODE_MAX) {
-            monitor_printf(mon, "invalid parameter: %s\n", keyname_buf);
-            break;
-        }
-
         keylist = g_malloc0(sizeof(*keylist));
-        keylist->value = idx;
-        keylist->next = NULL;
+        keylist->value = g_malloc0(sizeof(*keylist->value));
 
         if (!head) {
             head = keylist;
@@ -1147,17 +1140,39 @@ void hmp_send_key(Monitor *mon, const QDict *qdict)
         }
         tmp = keylist;
 
+        if (strstart(keyname_buf, "0x", NULL)) {
+            char *endp;
+            int value = strtoul(keyname_buf, &endp, 0);
+            if (*endp != '\0') {
+                goto err_out;
+            }
+            keylist->value->kind = KEY_VALUE_KIND_HEX;
+            keylist->value->hex = value;
+        } else {
+            int idx = index_from_key(keyname_buf);
+            if (idx == Q_KEY_CODE_MAX) {
+                goto err_out;
+            }
+            keylist->value->kind = KEY_VALUE_KIND_QCODE;
+            keylist->value->qcode = idx;
+        }
+
         if (!separator) {
             break;
         }
         keys = separator + 1;
     }
 
-    if (idx != Q_KEY_CODE_MAX) {
-        qmp_send_key(head, has_hold_time, hold_time, &err);
-    }
+    qmp_send_key(head, has_hold_time, hold_time, &err);
     hmp_handle_error(mon, &err);
-    qapi_free_QKeyCodeList(head);
+
+out:
+    qapi_free_KeyValueList(head);
+    return;
+
+err_out:
+    monitor_printf(mon, "invalid parameter: %s\n", keyname_buf);
+    goto out;
 }
 
 void hmp_screen_dump(Monitor *mon, const QDict *qdict)
diff --git a/input.c b/input.c
index 32c6057..187a131 100644
--- a/input.c
+++ b/input.c
@@ -228,6 +228,16 @@ static int *keycodes;
 static int keycodes_size;
 static QEMUTimer *key_timer;
 
+static int keycode_from_keyvalue(const KeyValue *value)
+{
+    if (value->kind == KEY_VALUE_KIND_QCODE) {
+        return key_defs[value->qcode];
+    } else {
+        assert(value->kind == KEY_VALUE_KIND_HEX);
+        return value->hex;
+    }
+}
+
 static void release_keys(void *opaque)
 {
     int i;
@@ -244,11 +254,11 @@ static void release_keys(void *opaque)
     keycodes_size = 0;
 }
 
-void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
+void qmp_send_key(KeyValueList *keys, bool has_hold_time, int64_t hold_time,
                   Error **errp)
 {
     int keycode;
-    QKeyCodeList *p;
+    KeyValueList *p;
 
     if (!key_timer) {
         key_timer = qemu_new_timer_ns(vm_clock, release_keys, NULL);
@@ -265,7 +275,14 @@ void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
 
     for (p = keys; p != NULL; p = p->next) {
         /* key down events */
-        keycode = key_defs[p->value];
+        keycode = keycode_from_keyvalue(p->value);
+        if (keycode < 0x01 || keycode > 0xff) {
+            error_setg(errp, "invalid hex keycode 0x%x\n", keycode);
+            g_free(keycodes);
+            keycodes_size = 0;
+            return;
+        }
+
         if (keycode & 0x80) {
             kbd_put_keycode(0xe0);
         }
diff --git a/qapi-schema.json b/qapi-schema.json
index 14e4419..688230f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2588,12 +2588,25 @@
              'lf', 'help', 'meta_l', 'meta_r', 'compose' ] }
 
 ##
+# @KeyValue
+#
+# Represents a keyboard key.
+# 
+# Since: 1.3.0
+##
+{ 'union': 'KeyValue',
+  'data': {
+    'hex': 'int',
+    'qcode': 'QKeyCode' } }
+
+##
 # @send-key:
 #
 # Send keys to guest.
 #
-# @keys: key sequence. 'keys' is the name of the key. Use a JSON array to
-#        press several keys simultaneously.
+# @keys: An array of @KeyValue elements. All @KeyValues in this array are
+#        simultaneously sent to the guest. A @KeyValue.hex value is sent
+#        directly to the guest
 #
 # @hold-time: #optional time to delay key up events, milliseconds. Defaults
 #             to 100
@@ -2605,7 +2618,7 @@
 #
 ##
 { 'command': 'send-key',
-  'data': { 'keys': ['QKeyCode'], '*hold-time': 'int' } }
+  'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
 
 ##
 # @screendump:
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 3/3] input: index_from_key(): drop unused code

[Luiz Capitulino]

The hex key conversion is unused since last commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 input.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/input.c b/input.c
index 187a131..da3ed27 100644
--- a/input.c
+++ b/input.c
@@ -186,8 +186,7 @@ static const int key_defs[] = {
 
 int index_from_key(const char *key)
 {
-    int i, keycode;
-    char *endp;
+    int i;
 
     for (i = 0; QKeyCode_lookup[i] != NULL; i++) {
         if (!strcmp(key, QKeyCode_lookup[i])) {
@@ -195,17 +194,6 @@ int index_from_key(const char *key)
         }
     }
 
-    if (strstart(key, "0x", NULL)) {
-        keycode = strtoul(key, &endp, 0);
-        if (*endp == '\0' && keycode >= 0x01 && keycode <= 0xff) {
-            for (i = 0; i < Q_KEY_CODE_MAX; i++) {
-                if (keycode == key_defs[i]) {
-                    break;
-                }
-            }
-        }
-    }
-
     /* Return Q_KEY_CODE_MAX if the key is invalid */
     return i;
 }
-- 
1.7.12.315.g682ce8b

Re: [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]

On 09/20/2012 12:18 PM, Luiz Capitulino wrote:
> Before the qapi conversion, the sendkey command could be used to
> send key codes in hex directly to the guest. This doesn't work
> with the current implemention, as it will only send to the guest
> QKeyCode values. That's a regression.
> 
> This commit fixes the problem by adding hex value support down
> the QMP interface, qmp_send_key().
> 
> In more detail, this commit:
> 
>  1. Adds the KeyValue union. This can represent an hex value or
>     a QKeyCode value
> 
>  2. *Changes* the QMP send-key command to take an KeyValue argument
>     instead of a QKeyCode one.
> 
>  3. Adapt hmp_send_key() to the QMP interface changes
> 
> Item 2 is an incompatible change, but as we're development phase
> (and this command has been merged a few weeks ago) this shouldn't be
> a problem.

Concur - send-key is listed as 'Since: 1.3.0', so we are free to change
it however we see fit.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

[Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify

[Luiz Capitulino]

The current code duplicates the QKeyCodeList keys in order to store
the key values for release_keys() late run. This is a bit complicated
though, as we have to care about correct ordering and then release_keys()
will have to index key_defs[] over again.

Switch to an array of integers, which is dynamically allocated and stores
the already converted key value.

This simplifies the current code and the next commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 input.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/input.c b/input.c
index c4b0619..32c6057 100644
--- a/input.c
+++ b/input.c
@@ -224,30 +224,31 @@ int index_from_keycode(int code)
     return i;
 }
 
-static QKeyCodeList *keycodes;
+static int *keycodes;
+static int keycodes_size;
 static QEMUTimer *key_timer;
 
 static void release_keys(void *opaque)
 {
-    int keycode;
-    QKeyCodeList *p;
+    int i;
 
-    for (p = keycodes; p != NULL; p = p->next) {
-        keycode = key_defs[p->value];
-        if (keycode & 0x80) {
+    for (i = 0; i < keycodes_size; i++) {
+        if (keycodes[i] & 0x80) {
             kbd_put_keycode(0xe0);
         }
-        kbd_put_keycode(keycode | 0x80);
+        kbd_put_keycode(keycodes[i]| 0x80);
     }
-    qapi_free_QKeyCodeList(keycodes);
+
+    g_free(keycodes);
     keycodes = NULL;
+    keycodes_size = 0;
 }
 
 void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
                   Error **errp)
 {
     int keycode;
-    QKeyCodeList *p, *keylist, *head = NULL, *tmp = NULL;
+    QKeyCodeList *p;
 
     if (!key_timer) {
         key_timer = qemu_new_timer_ns(vm_clock, release_keys, NULL);
@@ -257,31 +258,22 @@ void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
         qemu_del_timer(key_timer);
         release_keys(NULL);
     }
+
     if (!has_hold_time) {
         hold_time = 100;
     }
 
     for (p = keys; p != NULL; p = p->next) {
-        keylist = g_malloc0(sizeof(*keylist));
-        keylist->value = p->value;
-        keylist->next = NULL;
-
-        if (!head) {
-            head = keylist;
-        }
-        if (tmp) {
-            tmp->next = keylist;
-        }
-        tmp = keylist;
-
         /* key down events */
         keycode = key_defs[p->value];
         if (keycode & 0x80) {
             kbd_put_keycode(0xe0);
         }
         kbd_put_keycode(keycode & 0x7f);
+
+        keycodes = g_realloc(keycodes, sizeof(int) * (keycodes_size + 1));
+        keycodes[keycodes_size++] = keycode;
     }
-    keycodes = head;
 
     /* delayed key up events */
     qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) +
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify

[Luiz Capitulino]

The current code duplicates the QKeyCodeList keys in order to store
the key values for release_keys() late run. This is a bit complicated
though, as we have to care about correct ordering and then release_keys()
will have to index key_defs[] over again.

Switch to an array of integers, which is dynamically allocated and stores
the already converted key value.

This simplifies the current code and the next commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 input.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/input.c b/input.c
index c4b0619..32c6057 100644
--- a/input.c
+++ b/input.c
@@ -224,30 +224,31 @@ int index_from_keycode(int code)
     return i;
 }
 
-static QKeyCodeList *keycodes;
+static int *keycodes;
+static int keycodes_size;
 static QEMUTimer *key_timer;
 
 static void release_keys(void *opaque)
 {
-    int keycode;
-    QKeyCodeList *p;
+    int i;
 
-    for (p = keycodes; p != NULL; p = p->next) {
-        keycode = key_defs[p->value];
-        if (keycode & 0x80) {
+    for (i = 0; i < keycodes_size; i++) {
+        if (keycodes[i] & 0x80) {
             kbd_put_keycode(0xe0);
         }
-        kbd_put_keycode(keycode | 0x80);
+        kbd_put_keycode(keycodes[i]| 0x80);
     }
-    qapi_free_QKeyCodeList(keycodes);
+
+    g_free(keycodes);
     keycodes = NULL;
+    keycodes_size = 0;
 }
 
 void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
                   Error **errp)
 {
     int keycode;
-    QKeyCodeList *p, *keylist, *head = NULL, *tmp = NULL;
+    QKeyCodeList *p;
 
     if (!key_timer) {
         key_timer = qemu_new_timer_ns(vm_clock, release_keys, NULL);
@@ -257,31 +258,22 @@ void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
         qemu_del_timer(key_timer);
         release_keys(NULL);
     }
+
     if (!has_hold_time) {
         hold_time = 100;
     }
 
     for (p = keys; p != NULL; p = p->next) {
-        keylist = g_malloc0(sizeof(*keylist));
-        keylist->value = p->value;
-        keylist->next = NULL;
-
-        if (!head) {
-            head = keylist;
-        }
-        if (tmp) {
-            tmp->next = keylist;
-        }
-        tmp = keylist;
-
         /* key down events */
         keycode = key_defs[p->value];
         if (keycode & 0x80) {
             kbd_put_keycode(0xe0);
         }
         kbd_put_keycode(keycode & 0x7f);
+
+        keycodes = g_realloc(keycodes, sizeof(int) * (keycodes_size + 1));
+        keycodes[keycodes_size++] = keycode;
     }
-    keycodes = head;
 
     /* delayed key up events */
     qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) +
-- 
1.7.12.315.g682ce8b

Re: [Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> The current code duplicates the QKeyCodeList keys in order to store
> the key values for release_keys() late run. This is a bit complicated
> though, as we have to care about correct ordering and then release_keys()
> will have to index key_defs[] over again.
>
> Switch to an array of integers, which is dynamically allocated and stores
> the already converted key value.
>
> This simplifies the current code and the next commit.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  input.c | 36 ++++++++++++++----------------------
>  1 file changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/input.c b/input.c
> index c4b0619..32c6057 100644
> --- a/input.c
> +++ b/input.c
> @@ -224,30 +224,31 @@ int index_from_keycode(int code)
>      return i;
>  }
>  
> -static QKeyCodeList *keycodes;
> +static int *keycodes;
> +static int keycodes_size;
>  static QEMUTimer *key_timer;
>  
>  static void release_keys(void *opaque)
>  {
> -    int keycode;
> -    QKeyCodeList *p;
> +    int i;
>  
> -    for (p = keycodes; p != NULL; p = p->next) {
> -        keycode = key_defs[p->value];
> -        if (keycode & 0x80) {
> +    for (i = 0; i < keycodes_size; i++) {
> +        if (keycodes[i] & 0x80) {
>              kbd_put_keycode(0xe0);
>          }
> -        kbd_put_keycode(keycode | 0x80);
> +        kbd_put_keycode(keycodes[i]| 0x80);
>      }
> -    qapi_free_QKeyCodeList(keycodes);
> +
> +    g_free(keycodes);
>      keycodes = NULL;
> +    keycodes_size = 0;
>  }
>  
>  void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
>                    Error **errp)
>  {
>      int keycode;
> -    QKeyCodeList *p, *keylist, *head = NULL, *tmp = NULL;
> +    QKeyCodeList *p;
>  
>      if (!key_timer) {
>          key_timer = qemu_new_timer_ns(vm_clock, release_keys, NULL);
> @@ -257,31 +258,22 @@ void qmp_send_key(QKeyCodeList *keys, bool has_hold_time, int64_t hold_time,
>          qemu_del_timer(key_timer);
>          release_keys(NULL);
>      }
> +
>      if (!has_hold_time) {
>          hold_time = 100;
>      }
>  
>      for (p = keys; p != NULL; p = p->next) {
> -        keylist = g_malloc0(sizeof(*keylist));
> -        keylist->value = p->value;
> -        keylist->next = NULL;
> -
> -        if (!head) {
> -            head = keylist;
> -        }
> -        if (tmp) {
> -            tmp->next = keylist;
> -        }
> -        tmp = keylist;
> -
>          /* key down events */
>          keycode = key_defs[p->value];
>          if (keycode & 0x80) {
>              kbd_put_keycode(0xe0);
>          }
>          kbd_put_keycode(keycode & 0x7f);
> +
> +        keycodes = g_realloc(keycodes, sizeof(int) * (keycodes_size + 1));
> +        keycodes[keycodes_size++] = keycode;

One realloc per key is a bit wasteful (the common efficient way to grow
a flexible array is to double it), but it takes a mighty fast typist to
make that matter.

>      }
> -    keycodes = head;
>  
>      /* delayed key up events */
>      qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) +