| Message ID | 1348752509-1142-14-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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) (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. > 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> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Markus Armbruster <armbru@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 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. ==> 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" } } > + '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: >
On 09/28/2012 04:36 AM, Amos Kong wrote: > On 27/09/12 21:28, Luiz Capitulino wrote: >> > > Sorry the delay review. > > > hex isn't support when using qmp monitor. > >> +{ 'union': 'KeyValue', >> + 'data': { >> + 'number': 'int', > > It's 'int' not hex format. Indeed - I just re-read the JSON overview at http://www.json.org/, which is explicit that numbers are decimal only (no octal or hexadecimal support, and no support for a leading 0 except when the number is exactly 0). [Serves me right for not realizing this aspect of JSON when I did my review earlier.] I don't think this invalidates the QMP (libvirt already sends decimal, and wasn't planning on sending hex), but you DO have a point that: > > qmp-commands.hx also needs to be updated with latest examples: Since this is already in the PULL request, I think the docs touchup could be a separate patch.
On Fri, 28 Sep 2012 18:36:53 +0800 Amos Kong <akong@redhat.com> 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. > (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 <avi@redhat.com> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Markus Armbruster <armbru@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 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.
----- Original Message ----- > On Fri, 28 Sep 2012 18:36:53 +0800 > Amos Kong <akong@redhat.com> 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 <avi@redhat.com> > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > Reviewed-by: Markus Armbruster <armbru@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 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 <akong@redhat.com>
On Fri, 28 Sep 2012 07:01:31 -0600 Eric Blake <eblake@redhat.com> wrote: > On 09/28/2012 04:36 AM, Amos Kong wrote: > > On 27/09/12 21:28, Luiz Capitulino wrote: > >> > > > > Sorry the delay review. > > > > > > > hex isn't support when using qmp monitor. > > > > >> +{ 'union': 'KeyValue', > >> + 'data': { > >> + 'number': 'int', > > > > It's 'int' not hex format. > > Indeed - I just re-read the JSON overview at http://www.json.org/, which > is explicit that numbers are decimal only (no octal or hexadecimal > support, and no support for a leading 0 except when the number is > exactly 0). [Serves me right for not realizing this aspect of JSON when > I did my review earlier.] Sorry if I wasn't clear or if my use of "hex" caused confusion, but it's known that the only way of having an hex value on the wire is to make it a string. As the base number we use on the wire is pretty irrelevant (at least for this case) I don't see any issue here (actually, for json this is pretty natural). > I don't think this invalidates the QMP (libvirt already sends decimal, > and wasn't planning on sending hex), but you DO have a point that: > > > > > qmp-commands.hx also needs to be updated with latest examples: > > Since this is already in the PULL request, I think the docs touchup > could be a separate patch. Yes.
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', + '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:
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> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> --- hmp.c | 43 +++++++++++++++++++++++++++++-------------- input.c | 33 +++++++++++++++++++++++++++------ qapi-schema.json | 20 +++++++++++++++++--- 3 files changed, 73 insertions(+), 23 deletions(-)