From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60105) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THDdj-0000mH-3j for qemu-devel@nongnu.org; Thu, 27 Sep 2012 08:56:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1THDdc-0000pX-GS for qemu-devel@nongnu.org; Thu, 27 Sep 2012 08:56:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17676) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THDdc-0000pT-50 for qemu-devel@nongnu.org; Thu, 27 Sep 2012 08:56:20 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q8RCuJtg031446 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 27 Sep 2012 08:56:19 -0400 Date: Thu, 27 Sep 2012 09:57:11 -0300 From: Luiz Capitulino Message-ID: <20120927095711.3070fcac@doriath.home> In-Reply-To: <87zk4beloe.fsf@blackfin.pond.sub.org> References: <1348691138-12879-1-git-send-email-lcapitulino@redhat.com> <1348691138-12879-3-git-send-email-lcapitulino@redhat.com> <87zk4beloe.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] qmp: qmp_send_key(): accept key codes in hex List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: akong@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, avi@redhat.com On Thu, 27 Sep 2012 13:42:57 +0200 Markus Armbruster wrote: > Luiz Capitulino 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 > > Signed-off-by: Luiz Capitulino > > --- > > 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: >