From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46656) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THajI-0002JF-QA for qemu-devel@nongnu.org; Fri, 28 Sep 2012 09:35:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1THaj9-00035e-9H for qemu-devel@nongnu.org; Fri, 28 Sep 2012 09:35:44 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:42766) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THaj8-00035G-UU for qemu-devel@nongnu.org; Fri, 28 Sep 2012 09:35:35 -0400 Date: Fri, 28 Sep 2012 09:30:03 -0400 (EDT) From: Amos Kong Message-ID: <1130455458.3649249.1348839003821.JavaMail.root@redhat.com> In-Reply-To: <20120928102402.0732e35a@doriath.home> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 13/15] qmp: qmp_send_key(): accept key codes in hex List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org ----- Original Message ----- > On Fri, 28 Sep 2012 18:36:53 +0800 > Amos Kong wrote: > > > On 27/09/12 21:28, Luiz Capitulino wrote: > > > > > > > Sorry the delay review. > > > > > 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 > > > > '0xdc' is not a available keycode, I didn't find '0xdc' in old > > key_defs[] > > (before my send-key series, commit: > > 8db972cfa469b4e4afd9c65e54e796b83b5ce3a2) > > Yes, that's the point of this series. Before the qapi conversion the > sendkey command would send "unknown" key codes such as 0xdc directly > to the guest. The conversion just dropped that feature by rejecting > such key codes. Got it. > > (latest upstream code: 6f8fd2530e9a530f237240daf1c981fa5df7f978) > > (qemu) sendkey 0x10 > > 'q' will be inputted to guest > > (qemu) sendkey 0x1d-0x38-0xd3 > > guest will be reset > > > > hex isn't support when using qmp monitor. > > The HMP monitor is just a UI on top of qmp. It's the qmp interface > that has to be changed in order to support sending key codes > directly to the guest. > > > > 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 > > > Signed-off-by: Luiz Capitulino > > > Reviewed-by: Eric Blake > > > Reviewed-by: Markus Armbruster > > > --- > > > 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 2de3140..3306bcd 100644 > > > --- a/hmp.c > > > +++ b/hmp.c > > > @@ -1113,13 +1113,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, '-'); > > > @@ -1133,15 +1133,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; > > > @@ -1151,17 +1144,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 c6a6767..28d8815 100644 > > > --- a/qapi-schema.json > > > +++ b/qapi-schema.json > > > @@ -2621,12 +2621,26 @@ > > > 'lf', 'help', 'meta_l', 'meta_r', 'compose' ] } > > > > > > ## > > > +# @KeyValue > > > +# > > > +# Represents a keyboard key. > > > +# > > > +# Since: 1.3.0 > > > +## > > > +{ 'union': 'KeyValue', > > > + 'data': { > > > + 'number': 'int', > > > > It's 'int' not hex format. > > On the wire we support decimal numbers, and on the C interface > "number" can have any base. > > The way I used "hex" to describe this feature might be a bit > confusing, > as I mostly had in mind HMP (which does use hex format) and the QMP C > interface (where you usually want to specify "number" in hex). > > But that's just a minor terminology confusion, there's absolutely no > changes in the feature's semantics. > > > ==> works with 'int' > > { "execute": "send-key", "arguments": { 'keys': [{'type':'hex', > > 'data': > > 29}, {'type':'hex', 'data': 56}, {'type':'hex', 'data': 211}]}} > > { > > "return": { > > } > > } > > { > > "timestamp": { > > "seconds": 1348826804, > > "microseconds": 777545 > > }, > > "event": "RESET" > > } > > > > ==> doesn't work with 'hex' > > { "execute": "send-key", "arguments": { 'keys': [{'type':'hex', > > 'data': > > 0x1d}, {'type':'hex', 'data': 0x38}, {'type':'hex', 'data': > > 0xd3}]}} > > { > > "error": { > > "class": "GenericError", > > "desc": "Invalid JSON syntax" > > } > > } > > That's correct, you should use decimal numbers on the wire. > > > > + '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 > > > @@ -2638,7 +2652,7 @@ > > > # > > > ## > > > { 'command': 'send-key', > > > - 'data': { 'keys': ['QKeyCode'], '*hold-time': 'int' } } > > > + 'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } } > > > > > > ## > > > # @screendump: > > > > > > qmp-commands.hx also needs to be updated with latest examples: > > Good catch, but that can be done on a follow up patch. Ok, according to your comments, the only issue is doc. Acked-by: Amos Kong