[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 13/15] qmp: qmp_send_key(): accept key codes in h
From: |
Amos Kong |
Subject: |
Re: [Qemu-devel] [PULL 13/15] qmp: qmp_send_key(): accept key codes in hex |
Date: |
Fri, 28 Sep 2012 09:30:03 -0400 (EDT) |
----- Original Message -----
> On Fri, 28 Sep 2012 18:36:53 +0800
> Amos Kong <address@hidden> 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 <address@hidden>
> > > Signed-off-by: Luiz Capitulino <address@hidden>
> > > Reviewed-by: Eric Blake <address@hidden>
> > > Reviewed-by: Markus Armbruster <address@hidden>
> > > ---
> > > 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 <address@hidden>
- [Qemu-devel] [PULL 02/15] Support settimeout in QEMUMonitorProtocol, (continued)
- [Qemu-devel] [PULL 02/15] Support settimeout in QEMUMonitorProtocol, Luiz Capitulino, 2012/09/27
- [Qemu-devel] [PULL 08/15] qapi: convert add_client, Luiz Capitulino, 2012/09/27
- [Qemu-devel] [PULL 07/15] monitor: add Error * argument to monitor_get_fd, Luiz Capitulino, 2012/09/27
- [Qemu-devel] [PULL 11/15] hmp: dump-guest-memory: hardcode protocol argument to "file:", Luiz Capitulino, 2012/09/27
- [Qemu-devel] [PULL 09/15] qmp: dump-guest-memory: improve schema doc (again), Luiz Capitulino, 2012/09/27
- [Qemu-devel] [PULL 13/15] qmp: qmp_send_key(): accept key codes in hex, Luiz Capitulino, 2012/09/27
[Qemu-devel] [PULL 10/15] qmp: dump-guest-memory: don't spin if non-blocking fd would block, Luiz Capitulino, 2012/09/27
[Qemu-devel] [PULL 14/15] input: index_from_key(): drop unused code, Luiz Capitulino, 2012/09/27
[Qemu-devel] [PULL 12/15] input: qmp_send_key(): simplify, Luiz Capitulino, 2012/09/27
[Qemu-devel] [PULL 15/15] block: live snapshot documentation tweaks, Luiz Capitulino, 2012/09/27