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

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

[Luiz Capitulino]

o v2

 - re-write some commit logs
 - fix a possible issue with error handling in patch 2/3

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

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

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

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

[Luiz Capitulino]

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

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

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

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 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 --]

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

[Eric Blake]

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

On 09/21/2012 08:55 AM, Luiz Capitulino wrote:
> o v2
> 
>  - re-write some commit logs
>  - fix a possible issue with error handling in patch 2/3
> 
> This actually fixes a regression introduced by the qapi conversion,
> please refer to patch 2/3 for details.
> 
> It's also important to note that this series changes the QMP
> interface for the send-key command, but this shouldn't be a problem
> as we're still in development phase.
> 
>  hmp.c            | 43 +++++++++++++++++++++-----------
>  input.c          | 75 ++++++++++++++++++++++++++++----------------------------
>  qapi-schema.json | 19 +++++++++++---
>  3 files changed, 83 insertions(+), 54 deletions(-)

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

However, I have a bikeshed question on patch 2 that may require a respin.

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

[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

[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

[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

[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

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