* [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex
@ 2012-09-26 20:25 Luiz Capitulino
2012-09-26 20:25 ` [Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify Luiz Capitulino
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Luiz Capitulino @ 2012-09-26 20:25 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, eblake, avi, armbru
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify
2012-09-26 20:25 [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Luiz Capitulino
@ 2012-09-26 20:25 ` Luiz Capitulino
2012-09-27 9:50 ` Markus Armbruster
2012-09-26 20:25 ` [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex Luiz Capitulino
` (4 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Luiz Capitulino @ 2012-09-26 20:25 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, eblake, avi, armbru
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex
2012-09-26 20:25 [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Luiz Capitulino
2012-09-26 20:25 ` [Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify Luiz Capitulino
@ 2012-09-26 20:25 ` Luiz Capitulino
2012-09-27 11:42 ` Markus Armbruster
2012-09-26 20:25 ` [Qemu-devel] [PATCH 3/3] input: index_from_key(): drop unused code Luiz Capitulino
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Luiz Capitulino @ 2012-09-26 20:25 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, eblake, avi, armbru
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/3] input: index_from_key(): drop unused code
2012-09-26 20:25 [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Luiz Capitulino
2012-09-26 20:25 ` [Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify Luiz Capitulino
2012-09-26 20:25 ` [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex Luiz Capitulino
@ 2012-09-26 20:25 ` Luiz Capitulino
2012-09-26 21:23 ` [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Eric Blake
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Luiz Capitulino @ 2012-09-26 20:25 UTC (permalink / raw)
To: qemu-devel; +Cc: akong, eblake, avi, armbru
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex
2012-09-26 20:25 [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Luiz Capitulino
` (2 preceding siblings ...)
2012-09-26 20:25 ` [Qemu-devel] [PATCH 3/3] input: index_from_key(): drop unused code Luiz Capitulino
@ 2012-09-26 21:23 ` Eric Blake
2012-09-27 9:19 ` Avi Kivity
2012-09-27 11:43 ` Markus Armbruster
5 siblings, 0 replies; 11+ messages in thread
From: Eric Blake @ 2012-09-26 21:23 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: armbru, akong, qemu-devel, avi
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex
2012-09-26 20:25 [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Luiz Capitulino
` (3 preceding siblings ...)
2012-09-26 21:23 ` [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Eric Blake
@ 2012-09-27 9:19 ` Avi Kivity
2012-09-27 12:14 ` Luiz Capitulino
2012-09-27 11:43 ` Markus Armbruster
5 siblings, 1 reply; 11+ messages in thread
From: Avi Kivity @ 2012-09-27 9:19 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, eblake, qemu-devel, Anthony Liguori, armbru
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify
2012-09-26 20:25 ` [Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify Luiz Capitulino
@ 2012-09-27 9:50 ` Markus Armbruster
0 siblings, 0 replies; 11+ messages in thread
From: Markus Armbruster @ 2012-09-27 9:50 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, eblake, qemu-devel, avi
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) +
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex
2012-09-26 20:25 ` [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex Luiz Capitulino
@ 2012-09-27 11:42 ` Markus Armbruster
2012-09-27 12:57 ` Luiz Capitulino
0 siblings, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2012-09-27 11:42 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, eblake, qemu-devel, avi
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:
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex
2012-09-26 20:25 [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Luiz Capitulino
` (4 preceding siblings ...)
2012-09-27 9:19 ` Avi Kivity
@ 2012-09-27 11:43 ` Markus Armbruster
5 siblings, 0 replies; 11+ messages in thread
From: Markus Armbruster @ 2012-09-27 11:43 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: akong, eblake, qemu-devel, avi
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex
2012-09-27 9:19 ` Avi Kivity
@ 2012-09-27 12:14 ` Luiz Capitulino
0 siblings, 0 replies; 11+ messages in thread
From: Luiz Capitulino @ 2012-09-27 12:14 UTC (permalink / raw)
To: Avi Kivity; +Cc: akong, eblake, qemu-devel, Anthony Liguori, armbru
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex
2012-09-27 11:42 ` Markus Armbruster
@ 2012-09-27 12:57 ` Luiz Capitulino
0 siblings, 0 replies; 11+ messages in thread
From: Luiz Capitulino @ 2012-09-27 12:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: akong, eblake, qemu-devel, avi
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:
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-09-27 12:56 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-26 20:25 [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Luiz Capitulino
2012-09-26 20:25 ` [Qemu-devel] [PATCH 1/3] input: qmp_send_key(): simplify Luiz Capitulino
2012-09-27 9:50 ` Markus Armbruster
2012-09-26 20:25 ` [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex Luiz Capitulino
2012-09-27 11:42 ` Markus Armbruster
2012-09-27 12:57 ` Luiz Capitulino
2012-09-26 20:25 ` [Qemu-devel] [PATCH 3/3] input: index_from_key(): drop unused code Luiz Capitulino
2012-09-26 21:23 ` [Qemu-devel] [PATCH v3 0/3] qmp: send-key: accept key codes in hex Eric Blake
2012-09-27 9:19 ` Avi Kivity
2012-09-27 12:14 ` Luiz Capitulino
2012-09-27 11:43 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.