Patchwork [2/3] qmp: qmp_send_key(): accept key codes in hex

login
register
mail settings
Submitter Luiz Capitulino
Date Sept. 21, 2012, 2:55 p.m.
Message ID <1348239340-29021-3-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/185790/
State New
Headers show

Comments

Luiz Capitulino - Sept. 21, 2012, 2:55 p.m.
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(-)
Eric Blake - Sept. 21, 2012, 4:31 p.m.
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?
Luiz Capitulino - Sept. 21, 2012, 4:42 p.m.
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.
Luiz Capitulino - Sept. 21, 2012, 5:26 p.m.
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?
Eric Blake - Sept. 21, 2012, 6:18 p.m.
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 - Sept. 21, 2012, 6:20 p.m.
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).
Markus Armbruster - Sept. 21, 2012, 6:20 p.m.
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;
Avi Kivity - Sept. 23, 2012, 8:49 a.m.
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'?

Patch

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: