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

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

[Luiz Capitulino]

o v3

 - doc & log fixups
 - rename KeyCode.hex to KeyCode.number

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.

Luiz Capitulino (3):
  input: qmp_send_key(): simplify
  qmp: qmp_send_key(): accept key codes in hex
  input: index_from_key(): drop unused code

 hmp.c            | 43 +++++++++++++++++++++-----------
 input.c          | 75 ++++++++++++++++++++++++++++----------------------------
 qapi-schema.json | 20 ++++++++++++---
 3 files changed, 84 insertions(+), 54 deletions(-)

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

[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. In HMP, this would
be like:

 (qemu) sendkey 0xdc

However, the qapi conversion broke this, as it only supports sending
QKeyCode values to the guest. 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 in development phase
(and this command has been merged a few weeks ago) this shouldn't be
a problem.

Finally, 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          | 33 +++++++++++++++++++++++++++------
 qapi-schema.json | 20 +++++++++++++++++---
 3 files changed, 73 insertions(+), 23 deletions(-)

diff --git a/hmp.c b/hmp.c
index ba6fbd3..8738dc5 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_NUMBER;
+            keylist->value->number = 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..76ade64 100644
--- a/input.c
+++ b/input.c
@@ -228,6 +228,23 @@ 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_NUMBER);
+        return value->number;
+    }
+}
+
+static void free_keycodes(void)
+{
+    g_free(keycodes);
+    keycodes = NULL;
+    keycodes_size = 0;
+}
+
 static void release_keys(void *opaque)
 {
     int i;
@@ -239,16 +256,14 @@ static void release_keys(void *opaque)
         kbd_put_keycode(keycodes[i]| 0x80);
     }
 
-    g_free(keycodes);
-    keycodes = NULL;
-    keycodes_size = 0;
+    free_keycodes();
 }
 
-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 +280,13 @@ 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);
+            free_keycodes();
+            return;
+        }
+
         if (keycode & 0x80) {
             kbd_put_keycode(0xe0);
         }
diff --git a/qapi-schema.json b/qapi-schema.json
index 14e4419..02135cc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2588,12 +2588,26 @@
              'lf', 'help', 'meta_l', 'meta_r', 'compose' ] }
 
 ##
+# @KeyValue
+#
+# Represents a keyboard key.
+# 
+# Since: 1.3.0
+##
+{ 'union': 'KeyValue',
+  'data': {
+    'number': '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.number value is sent
+#        directly to the guest, while @KeyValue.qcode must be a valid
+#        @QKeyCode value
 #
 # @hold-time: #optional time to delay key up events, milliseconds. Defaults
 #             to 100
@@ -2605,7 +2619,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 76ade64..25d3973 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 v3 0/3] qmp: send-key: accept key codes in hex

[Eric Blake]

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

On 09/26/2012 02:25 PM, Luiz Capitulino wrote:
> o v3
> 
>  - doc & log fixups
>  - rename KeyCode.hex to KeyCode.number
> 
> 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.
> 
> Luiz Capitulino (3):
>   input: qmp_send_key(): simplify
>   qmp: qmp_send_key(): accept key codes in hex
>   input: index_from_key(): drop unused code
> 
>  hmp.c            | 43 +++++++++++++++++++++-----------
>  input.c          | 75 ++++++++++++++++++++++++++++----------------------------
>  qapi-schema.json | 20 ++++++++++++---
>  3 files changed, 84 insertions(+), 54 deletions(-)
> 

Series:
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 --]

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

[Avi Kivity]

On 09/26/2012 10:25 PM, Luiz Capitulino wrote:
> o v3
> 
>  - doc & log fixups
>  - rename KeyCode.hex to KeyCode.number
> 
> 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.
> 

Anthony, this regression breaks autotest, so please give this series
higher priority if possible.

-- 
error compiling committee.c: too many arguments to function

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) +

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Before the qapi conversion, the sendkey command could be used to
> send key codes in hex directly to the guest. In HMP, this would
> be like:
>
>  (qemu) sendkey 0xdc
>
> However, the qapi conversion broke this, as it only supports sending
> QKeyCode values to the guest. That's a regression.

Let me elaborate, to make sure I got it.

Before the QAPI conversion, sendkey used get_keycode() to convert key
substrings to integer key codes.  Source code:

    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;
    }

If @key is a recognized key name, return its (positive) code.
Else, if @key is a hexadecimal number between 1 and 0xff, return the
number.
Else, return -1.

Since the QAPI conversion, it uses index_from_key():

    int index_from_key(const char *key)
    {
        int i, keycode;
        char *endp;

        for (i = 0; QKeyCode_lookup[i] != NULL; i++) {
            if (!strcmp(key, QKeyCode_lookup[i])) {
                break;
            }
        }

        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;
    }

If @key is a recognized key name, return its (positive) code.
Else, if @key is a hexadecimal number between 1 and 0xff, and a
recognized key with that code exists, return the number.
Else, return -1.

The regression is *not* that the hexadecimal format isn't recognized
anymore, it's that numbers that don't have a key definition are now
rejected.

> 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 in development phase
> (and this command has been merged a few weeks ago) this shouldn't be
> a problem.
>
> Finally, 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          | 33 +++++++++++++++++++++++++++------
>  qapi-schema.json | 20 +++++++++++++++++---
>  3 files changed, 73 insertions(+), 23 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..8738dc5 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_NUMBER;
> +            keylist->value->number = 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;
> +        }
> +

TL;DR: this differs unnecessarily from the pre-QAPI code, which makes
the fix less obvious than it could be.  But I think it should work, and
unbreaking autotest quickly is more important than making this patch
perfect, so I'm *not* asking for a respin because of that.

Strings starting with "0x" are interpreted differently than before the
QAPI conversion.

Before:

    If @key is a recognized key name, use its (positive) code.
    Else, if @key is a hexadecimal number between 1 and 0xff, use the
    number.
    Else, error.

After:

    If @key starts with "0x"
        If @key is a number, use it (range check is done later)
        Else, error
    Else
        If it's a recognized key name, use its (positive) code
        Else, error.

Makes a difference only if key names starting with "0x" exist.  None
exist now, and such names aren't likely to appear in the future.

Aside: codes starting with a digit do exist, and that's why we can do
only hexadecimal numeric keys.

The use of strtoul() here is sloppy, but it's no worse as before, in
index_from_key():

* Numbers overflowing int are silently truncated, as before.

* Numbers outside 0x01..0xff (possibly after truncation) are still
  flagged as error, in qmp_send_key() below.

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

Jumping forward and backward like that is a bit ugly, but let's unbreak
autotest.

>  }
>  
>  void hmp_screen_dump(Monitor *mon, const QDict *qdict)
> diff --git a/input.c b/input.c
> index 32c6057..76ade64 100644
> --- a/input.c
> +++ b/input.c
> @@ -228,6 +228,23 @@ 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_NUMBER);
> +        return value->number;
> +    }
> +}
> +
> +static void free_keycodes(void)
> +{
> +    g_free(keycodes);
> +    keycodes = NULL;
> +    keycodes_size = 0;
> +}
> +
>  static void release_keys(void *opaque)
>  {
>      int i;
> @@ -239,16 +256,14 @@ static void release_keys(void *opaque)
>          kbd_put_keycode(keycodes[i]| 0x80);
>      }
>  
> -    g_free(keycodes);
> -    keycodes = NULL;
> -    keycodes_size = 0;
> +    free_keycodes();
>  }
>  
> -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 +280,13 @@ 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);
> +            free_keycodes();
> +            return;
> +        }
> +
>          if (keycode & 0x80) {
>              kbd_put_keycode(0xe0);
>          }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 14e4419..02135cc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2588,12 +2588,26 @@
>               'lf', 'help', 'meta_l', 'meta_r', 'compose' ] }
>  
>  ##
> +# @KeyValue
> +#
> +# Represents a keyboard key.
> +# 

Trailing whitespace, could be fixed up on merge if we care.

> +# Since: 1.3.0
> +##
> +{ 'union': 'KeyValue',
> +  'data': {
> +    'number': '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.number value is sent
> +#        directly to the guest, while @KeyValue.qcode must be a valid
> +#        @QKeyCode value
>  #
>  # @hold-time: #optional time to delay key up events, milliseconds. Defaults
>  #             to 100
> @@ -2605,7 +2619,7 @@
>  #
>  ##
>  { 'command': 'send-key',
> -  'data': { 'keys': ['QKeyCode'], '*hold-time': 'int' } }
> +  'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
>  
>  ##
>  # @screendump:

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> o v3
>
>  - doc & log fixups
>  - rename KeyCode.hex to KeyCode.number
>
> 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.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Luiz Capitulino]

On Thu, 27 Sep 2012 11:19:58 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 09/26/2012 10:25 PM, Luiz Capitulino wrote:
> > o v3
> > 
> >  - doc & log fixups
> >  - rename KeyCode.hex to KeyCode.number
> > 
> > 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.
> > 
> 
> Anthony, this regression breaks autotest, so please give this series
> higher priority if possible.

As my other in-flight series have already been reviewed, I'll send a pull
request today with all queued qmp patches I have.

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

[Luiz Capitulino]

On Thu, 27 Sep 2012 13:42:57 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Before the qapi conversion, the sendkey command could be used to
> > send key codes in hex directly to the guest. In HMP, this would
> > be like:
> >
> >  (qemu) sendkey 0xdc
> >
> > However, the qapi conversion broke this, as it only supports sending
> > QKeyCode values to the guest. That's a regression.
> 
> Let me elaborate, to make sure I got it.
> 
> Before the QAPI conversion, sendkey used get_keycode() to convert key
> substrings to integer key codes.  Source code:
> 
>     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;
>     }
> 
> If @key is a recognized key name, return its (positive) code.
> Else, if @key is a hexadecimal number between 1 and 0xff, return the
> number.
> Else, return -1.
> 
> Since the QAPI conversion, it uses index_from_key():
> 
>     int index_from_key(const char *key)
>     {
>         int i, keycode;
>         char *endp;
> 
>         for (i = 0; QKeyCode_lookup[i] != NULL; i++) {
>             if (!strcmp(key, QKeyCode_lookup[i])) {
>                 break;
>             }
>         }
> 
>         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;
>     }
> 
> If @key is a recognized key name, return its (positive) code.
> Else, if @key is a hexadecimal number between 1 and 0xff, and a
> recognized key with that code exists, return the number.
> Else, return -1.
> 
> The regression is *not* that the hexadecimal format isn't recognized
> anymore, it's that numbers that don't have a key definition are now
> rejected.

Yes.

> > 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 in development phase
> > (and this command has been merged a few weeks ago) this shouldn't be
> > a problem.
> >
> > Finally, 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          | 33 +++++++++++++++++++++++++++------
> >  qapi-schema.json | 20 +++++++++++++++++---
> >  3 files changed, 73 insertions(+), 23 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index ba6fbd3..8738dc5 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_NUMBER;
> > +            keylist->value->number = 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;
> > +        }
> > +
> 
> TL;DR: this differs unnecessarily from the pre-QAPI code, which makes
> the fix less obvious than it could be.  But I think it should work, and
> unbreaking autotest quickly is more important than making this patch
> perfect, so I'm *not* asking for a respin because of that.
> 
> Strings starting with "0x" are interpreted differently than before the
> QAPI conversion.
> 
> Before:
> 
>     If @key is a recognized key name, use its (positive) code.
>     Else, if @key is a hexadecimal number between 1 and 0xff, use the
>     number.
>     Else, error.
> 
> After:
> 
>     If @key starts with "0x"
>         If @key is a number, use it (range check is done later)
>         Else, error
>     Else
>         If it's a recognized key name, use its (positive) code
>         Else, error.
> 
> Makes a difference only if key names starting with "0x" exist.  None
> exist now, and such names aren't likely to appear in the future.
> 
> Aside: codes starting with a digit do exist, and that's why we can do
> only hexadecimal numeric keys.
> 
> The use of strtoul() here is sloppy, but it's no worse as before, in
> index_from_key():
> 
> * Numbers overflowing int are silently truncated, as before.
> 
> * Numbers outside 0x01..0xff (possibly after truncation) are still
>   flagged as error, in qmp_send_key() below.
> 
> >          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;
> 
> Jumping forward and backward like that is a bit ugly, but let's unbreak
> autotest.
> 
> >  }
> >  
> >  void hmp_screen_dump(Monitor *mon, const QDict *qdict)
> > diff --git a/input.c b/input.c
> > index 32c6057..76ade64 100644
> > --- a/input.c
> > +++ b/input.c
> > @@ -228,6 +228,23 @@ 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_NUMBER);
> > +        return value->number;
> > +    }
> > +}
> > +
> > +static void free_keycodes(void)
> > +{
> > +    g_free(keycodes);
> > +    keycodes = NULL;
> > +    keycodes_size = 0;
> > +}
> > +
> >  static void release_keys(void *opaque)
> >  {
> >      int i;
> > @@ -239,16 +256,14 @@ static void release_keys(void *opaque)
> >          kbd_put_keycode(keycodes[i]| 0x80);
> >      }
> >  
> > -    g_free(keycodes);
> > -    keycodes = NULL;
> > -    keycodes_size = 0;
> > +    free_keycodes();
> >  }
> >  
> > -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 +280,13 @@ 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);
> > +            free_keycodes();
> > +            return;
> > +        }
> > +
> >          if (keycode & 0x80) {
> >              kbd_put_keycode(0xe0);
> >          }
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 14e4419..02135cc 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2588,12 +2588,26 @@
> >               'lf', 'help', 'meta_l', 'meta_r', 'compose' ] }
> >  
> >  ##
> > +# @KeyValue
> > +#
> > +# Represents a keyboard key.
> > +# 
> 
> Trailing whitespace, could be fixed up on merge if we care.

Fixed in my queue.

> 
> > +# Since: 1.3.0
> > +##
> > +{ 'union': 'KeyValue',
> > +  'data': {
> > +    'number': '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.number value is sent
> > +#        directly to the guest, while @KeyValue.qcode must be a valid
> > +#        @QKeyCode value
> >  #
> >  # @hold-time: #optional time to delay key up events, milliseconds. Defaults
> >  #             to 100
> > @@ -2605,7 +2619,7 @@
> >  #
> >  ##
> >  { 'command': 'send-key',
> > -  'data': { 'keys': ['QKeyCode'], '*hold-time': 'int' } }
> > +  'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
> >  
> >  ##
> >  # @screendump:
> 

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

[Avi Kivity]

On 09/21/2012 09:20 PM, Eric Blake wrote:
> On 09/21/2012 12:18 PM, Eric Blake wrote:
> >> Any better ideas?
> > 
> > Maybe s/hex/number/, as in:
> > 
> > JSON: 'number':0x20
> > 
> > C code: keylist->value->number = number;
> > 
> > that is, you are passing the value either as a keycode name, or as a number.
>
> Or even s/hex/int/
>
> except that the C code would then have to use mangling (since the JSON
> name would match a C keyword).
>

'keycode'?


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 21 Sep 2012 13:42:34 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>
>> On Fri, 21 Sep 2012 10:31:23 -0600
>> Eric Blake <eblake@redhat.com> wrote:
>> 
>> > On 09/21/2012 08:55 AM, Luiz Capitulino wrote:
>> > > This commit fixes the problem by adding hex value support down
>> > > the QMP interface, qmp_send_key().
>> > > 
>> > 
>> > > +++ 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',
>> > 
>> > Don't you find it a bit odd to name this 'hex', even though it works to
>> > do 'hex':32 as a synonym for 'hex':0x20?  Should we instead name it
>> > 'value', since we don't care in what base the value was represented,
>> > only that JSON was able to decode the base into a value?
>> 
>> Yes, that's a good point. I'll respin.
>
> Actually, this also has drawbacks:
>
>             keylist->value->kind = KEY_VALUE_KIND_VALUE;
>             keylist->value->value = value;
>
> Value, value, value... Value, value, value!
>
> I thought about renaming QKeyCode to KeyName and KeyValue to KeyCode, the
> problem though (apart from the code churn caused by the renaming) is that
> KeyName doesn't make much sense in C as it's all integers.
>
> Any better ideas?

keylist->value->kind = KEY_VALUE_KIND_NUM;
keylist->value->num = num;

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

[Eric Blake]

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

On 09/21/2012 12:18 PM, Eric Blake wrote:
>> Any better ideas?
> 
> Maybe s/hex/number/, as in:
> 
> JSON: 'number':0x20
> 
> C code: keylist->value->number = number;
> 
> that is, you are passing the value either as a keycode name, or as a number.

Or even s/hex/int/

except that the C code would then have to use mangling (since the JSON
name would match a C keyword).

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

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

[Eric Blake]

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

On 09/21/2012 11:26 AM, Luiz Capitulino wrote:
> On Fri, 21 Sep 2012 13:42:34 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 

>>>> +{ 'union': 'KeyValue',
>>>> +  'data': {
>>>> +    'hex': 'int',
     +    'qcode': 'QKeyCode',
>>>
>>> Don't you find it a bit odd to name this 'hex', even though it works to
>>> do 'hex':32 as a synonym for 'hex':0x20?  Should we instead name it
>>> 'value', since we don't care in what base the value was represented,
>>> only that JSON was able to decode the base into a value?
>>
>> Yes, that's a good point. I'll respin.
> 
> Actually, this also has drawbacks:
> 
>             keylist->value->kind = KEY_VALUE_KIND_VALUE;
>             keylist->value->value = value;

> Any better ideas?

Maybe s/hex/number/, as in:

JSON: 'number':0x20

C code: keylist->value->number = number;

that is, you are passing the value either as a keycode name, or as a number.

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

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

[Luiz Capitulino]

On Fri, 21 Sep 2012 13:42:34 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Fri, 21 Sep 2012 10:31:23 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 09/21/2012 08:55 AM, Luiz Capitulino wrote:
> > > This commit fixes the problem by adding hex value support down
> > > the QMP interface, qmp_send_key().
> > > 
> > 
> > > +++ 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',
> > 
> > Don't you find it a bit odd to name this 'hex', even though it works to
> > do 'hex':32 as a synonym for 'hex':0x20?  Should we instead name it
> > 'value', since we don't care in what base the value was represented,
> > only that JSON was able to decode the base into a value?
> 
> Yes, that's a good point. I'll respin.

Actually, this also has drawbacks:

            keylist->value->kind = KEY_VALUE_KIND_VALUE;
            keylist->value->value = value;

Value, value, value... Value, value, value!

I thought about renaming QKeyCode to KeyName and KeyValue to KeyCode, the
problem though (apart from the code churn caused by the renaming) is that
KeyName doesn't make much sense in C as it's all integers.

Any better ideas?

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

[Luiz Capitulino]

On Fri, 21 Sep 2012 10:31:23 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 09/21/2012 08:55 AM, Luiz Capitulino wrote:
> > This commit fixes the problem by adding hex value support down
> > the QMP interface, qmp_send_key().
> > 
> 
> > +++ 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',
> 
> Don't you find it a bit odd to name this 'hex', even though it works to
> do 'hex':32 as a synonym for 'hex':0x20?  Should we instead name it
> 'value', since we don't care in what base the value was represented,
> only that JSON was able to decode the base into a value?

Yes, that's a good point. I'll respin.

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

[Eric Blake]

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

On 09/21/2012 08:55 AM, Luiz Capitulino wrote:
> This commit fixes the problem by adding hex value support down
> the QMP interface, qmp_send_key().
> 

> +++ 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',

Don't you find it a bit odd to name this 'hex', even though it works to
do 'hex':32 as a synonym for 'hex':0x20?  Should we instead name it
'value', since we don't care in what base the value was represented,
only that JSON was able to decode the base into a value?

-- 
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 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. In HMP, this would
be like:

 (qemu) sendkey 0xdc

However, the qapi conversion broke this, 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 in development phase
(and this command has been merged a few weeks ago) this shouldn't be
a problem.

Finally, 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          | 33 +++++++++++++++++++++++++++------
 qapi-schema.json | 19 ++++++++++++++++---
 3 files changed, 72 insertions(+), 23 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..6be0231 100644
--- a/input.c
+++ b/input.c
@@ -228,6 +228,23 @@ 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 free_keycodes(void)
+{
+    g_free(keycodes);
+    keycodes = NULL;
+    keycodes_size = 0;
+}
+
 static void release_keys(void *opaque)
 {
     int i;
@@ -239,16 +256,14 @@ static void release_keys(void *opaque)
         kbd_put_keycode(keycodes[i]| 0x80);
     }
 
-    g_free(keycodes);
-    keycodes = NULL;
-    keycodes_size = 0;
+    free_keycodes();
 }
 
-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 +280,13 @@ 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);
+            free_keycodes();
+            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

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