[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

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

[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

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

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