On Wed, 27 Jun 2012 18:22:48 +0800
Amos Kong <ak...@redhat.com> wrote:
> On 27/06/12 04:21, Luiz Capitulino wrote:
> > On Wed, 20 Jun 2012 12:47:40 +0800
> > Amos Kong<ak...@redhat.com> wrote:
> >
> >> Convert 'sendkey' to use QAPI. do_sendkey() depends on some
> >> variables/functions in monitor.c, so reserve qmp_sendkey()
> >> to monitor.c
> >>
> >> key_defs[] in monitor.c is the mapping of key name to keycode,
> >> Keys' order in the enmu and key_defs[] is same.
> >>
> >> Signed-off-by: Amos Kong<ak...@redhat.com>
> >> ---
> >> hmp-commands.hx | 2 +-
> >> hmp.c | 45 ++++++++++++++++++++++++
> >> hmp.h | 1 +
> >> monitor.c | 102
> >> +++++++++++++++++++++++------------------------------
> >> monitor.h | 2 +
> >> qapi-schema.json | 45 ++++++++++++++++++++++++
> >> qmp-commands.hx | 27 ++++++++++++++
> >> 7 files changed, 165 insertions(+), 59 deletions(-)
> >>
> >> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> index e336251..fee4c14 100644
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -505,7 +505,7 @@ ETEXI
> >> .args_type = "keys:s,hold-time:i?",
> >> .params = "keys [hold_ms]",
> >> .help = "send keys to the VM (e.g. 'sendkey ctrl-alt-f1',
> >> default hold time=100 ms)",
> >> - .mhandler.cmd = do_sendkey,
> >> + .mhandler.cmd = hmp_sendkey,
> >> },
> >>
> >> STEXI
> >> diff --git a/hmp.c b/hmp.c
> >> index b9cec1d..846e8b9 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -19,6 +19,7 @@
> >> #include "qemu-timer.h"
> >> #include "qmp-commands.h"
> >> #include "monitor.h"
> >> +#include "qapi-types.h"
> >>
> >> static void hmp_handle_error(Monitor *mon, Error **errp)
> >> {
> >> @@ -1000,3 +1001,47 @@ void hmp_netdev_del(Monitor *mon, const QDict
> >> *qdict)
> >> qmp_netdev_del(id,&err);
> >> hmp_handle_error(mon,&err);
> >> }
> >> +
> >> +void hmp_sendkey(Monitor *mon, const QDict *qdict)
> >> +{
> >> + const char *keys = qdict_get_str(qdict, "keys");
> >> + KeyCodesList *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];
> >> +
> >
> > Please, drop this blank line.
> >
> >> + char *separator;
> >> + int keyname_len;
> >> +
> >> + while (1) {
> >> + separator = strchr(keys, '-');
> >> + keyname_len = separator ? separator - keys : strlen(keys);
> >> + pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
> >> +
> >> + /* Be compatible with old interface, convert user inputted "<" */
> >> + if (!strncmp(keyname_buf, "<", 1)&& keyname_len == 1) {
> >> + pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
> >> + keyname_len = 4;
> >> + }
> >> + keyname_buf[keyname_len] = 0;
> >> +
> >> + keylist = g_malloc0(sizeof(*keylist));
> >> + keylist->value = get_key_index(keyname_buf);
> >> + keylist->next = NULL;
> >> +
> >> + if (tmp)
> >> + tmp->next = keylist;
> >> + tmp = keylist;
> >> + if (!head)
> >> + head = keylist;
> >> +
> >> + if (!separator)
> >> + break;
> >> + keys = separator + 1;
> >> + }
> >> +
> >> + qmp_sendkey(head, has_hold_time, hold_time,&err);
> >> + hmp_handle_error(mon,&err);
> >> + qapi_free_KeyCodesList(head);
> >> +}
> >> diff --git a/hmp.h b/hmp.h
> >> index 79d138d..bcc74d2 100644
> >> --- a/hmp.h
> >> +++ b/hmp.h
> >> @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
> >> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
> >> void hmp_netdev_add(Monitor *mon, const QDict *qdict);
> >> void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> >> +void hmp_sendkey(Monitor *mon, const QDict *qdict);
> >>
> >> #endif
> >> diff --git a/monitor.c b/monitor.c
> >> index 6fa0104..863c0c6 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -1470,98 +1470,84 @@ static const KeyDef key_defs[] = {
> >> { 0, NULL },
> >> };
> >>
> >> -static int get_keycode(const char *key)
> >> +int get_key_index(const char *key)
> >> {
> >
> > This should be moved to hmp.c instead of making it public.
>
>
> The static 'key_defs' is used in this function,
> I tried to move key_defs definition to monitor.h,
> but I got this error:
>
> qemu/monitor.h:220:13: error: attempt to use poisoned "TARGET_SPARC"
> qemu/monitor.h:220:39: error: attempt to use poisoned "TARGET_SPARC64"
>
> Where is the good place to define key_defs ? it would be used in
> monitor.c & hmp.c
You could move it to input.c (whose functions are declared in console.h).
Actually, qmp_sendkey() should be moved there too.
But I'm now wondering if we're doing it right. The HMP interface is supposed
to be a real QMP client. However, to support hex keycodes it has to have
knowledge of the key mapping. Having access to key_defs if cheating, as hmp.c
is internal to qemu.
The two solutions I can think of are adding query-keymap or change
send-key to also accept hex values (which could be done through a new command).
Ideas?
> >> - const KeyDef *p;
> >> + int i, keycode;
> >> char *endp;
> >> - int ret;
> >> + const KeyDef *p;
> >>
> >> - for(p = key_defs; p->name != NULL; p++) {
> >> - if (!strcmp(key, p->name))
> >> - return p->keycode;
> >> + for (i = 0; KeyCodes_lookup[i] != NULL; i++) {
> >> + if (!strcmp(key, KeyCodes_lookup[i]))
> >> + return i;
> >> }
> >> +
> >> if (strstart(key, "0x", NULL)) {
> >> - ret = strtoul(key,&endp, 0);
> >> - if (*endp == '\0'&& ret>= 0x01&& ret<= 0xff)
> >> - return ret;
> >> + keycode = strtoul(key,&endp, 0);
> >> + if (*endp == '\0'&& keycode>= 0x01&& keycode<= 0xff)
> >> + i = 0;
> >> + for (p = key_defs; p->name != NULL; p++) {
> >> + if (keycode == p->keycode)
> >> + return i;
> >> + i++;
> >> + }
> >> }
> >> +
> >> return -1;
> >> }
> >>
> >> -#define MAX_KEYCODES 16
> >> -static uint8_t keycodes[MAX_KEYCODES];
> >> -static int nb_pending_keycodes;
> >> +static KeyCodesList *keycodes;
> >> static QEMUTimer *key_timer;
> >>
> >> static void release_keys(void *opaque)
> >> {
> >> int keycode;
> >>
> >> - while (nb_pending_keycodes> 0) {
> >> - nb_pending_keycodes--;
> >> - keycode = keycodes[nb_pending_keycodes];
> >> + while (keycodes != NULL) {
> >> + if (keycodes->value> sizeof(key_defs) / sizeof(*(key_defs))) {
> >
> > I don't think you need this check, see my comment on qmp_sendkey().
>
>
> >
> >> + keycodes = NULL;
> >> + break;
> >> + }
> >> +
> >> + keycode = key_defs[keycodes->value].keycode;
> >
> > I'm not sure I like this, as key_defs[] and the KeyCodes enum will have to
> > be
> > kept in sync and this can silently break. We could do:
> >
> > static const KeyDef key_defs[] = {
> > [KEY_CODES_SHIFT] = { .keycode = 0x2a, .name = "shift" },
> > };
> >
> > instead, to link the two tables.
> >
> > However, after this patch 'name' seems only to be used by the monitor
> > completion
> > code in monitor_find_completion(). So, maybe we could use KeyCodes_lookup[]
> > there
> > and drop 'name' and have an integer array to map the Keycodes enum to the
> > hex
> > values.
>
> I'll have a try.
>
>
> >> if (keycode& 0x80)
> >> kbd_put_keycode(0xe0);
> >> kbd_put_keycode(keycode | 0x80);
> >> +
> >> + keycodes = keycodes->next;
> >> }
> >> }
> >>
> >> -static void do_sendkey(Monitor *mon, const QDict *qdict)
> >> +void qmp_sendkey(KeyCodesList *keys, bool has_hold_time, int64_t
> >> hold_time,
> >> + Error **errp)
> >> {
> >> - char keyname_buf[16];
> >> - char *separator;
> >> - int keyname_len, keycode, i;
> >> - const char *keys = qdict_get_str(qdict, "keys");
> >> - int has_hold_time = qdict_haskey(qdict, "hold-time");
> >> - int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
> >> + int keycode;
> >> + char value[5];
> >>
> >> - if (nb_pending_keycodes> 0) {
> >> + if (keycodes != NULL) {
> >> qemu_del_timer(key_timer);
> >> release_keys(NULL);
> >> }
> >> if (!has_hold_time)
> >> hold_time = 100;
> >> - i = 0;
> >> - while (1) {
> >> - separator = strchr(keys, '-');
> >> - keyname_len = separator ? separator - keys : strlen(keys);
> >> - if (keyname_len> 0) {
> >> - pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
> >> - if (keyname_len> sizeof(keyname_buf) - 1) {
> >> - monitor_printf(mon, "invalid key: '%s...'\n",
> >> keyname_buf);
> >> - return;
> >> - }
> >> - if (i == MAX_KEYCODES) {
> >> - monitor_printf(mon, "too many keys\n");
> >> - return;
> >> - }
> >> -
> >> - /* Be compatible with old interface, convert user inputted
> >> "<" */
> >> - if (!strncmp(keyname_buf, "<", 1)&& keyname_len == 1) {
> >> - pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
> >> - keyname_len = 4;
> >> - }
> >>
> >> - keyname_buf[keyname_len] = 0;
> >> - keycode = get_keycode(keyname_buf);
> >> - if (keycode< 0) {
> >> - monitor_printf(mon, "unknown key: '%s'\n", keyname_buf);
> >> - return;
> >> - }
> >> - keycodes[i++] = keycode;
> >> + keycodes = keys;
> >> + while (keycodes != NULL) {
> >
> > I think that using a for loop is better.
>
>
> >> + if (keycodes->value> sizeof(key_defs) / sizeof(*(key_defs))) {
> >> + sprintf(value, "%d", keycodes->value);
> >> + error_set(errp, QERR_INVALID_PARAMETER, value);
> >> + return;
> >> }
> >
> > This is not necessary. The qapi will do this check, qmp_sendkey() won't be
> > called if there's an error.
>
> For qmp command, it will only pass valid keycode. but hmp_sendkey()
> might pass
> invalid parameters. Or we should check this in hmp_sendkey().
Good point, but then please use KEY_CODE_MAX.
>
>
> > Actually, our error message is buggy right now, I'll submit a patch.
> >
> >> - if (!separator)
> >> - break;
> >> - keys = separator + 1;
> >> - }
> >> - nb_pending_keycodes = i;
> >> - /* key down events */
> >> - for (i = 0; i< nb_pending_keycodes; i++) {
> >> - keycode = keycodes[i];
> >> +
> >> + /* key down events */
> >> + keycode = key_defs[keycodes->value].keycode;
> >> if (keycode& 0x80)
> >> kbd_put_keycode(0xe0);
> >> kbd_put_keycode(keycode& 0x7f);
> >> +
> >> + keycodes = keycodes->next;
> >> }
> >> + keycodes = keys;
> >> +
> >> /* delayed key up events */
> >> qemu_mod_timer(key_timer, qemu_get_clock_ns(vm_clock) +
> >> muldiv64(get_ticks_per_sec(), hold_time, 1000));
> >> diff --git a/monitor.h b/monitor.h
> >> index 5f4de1b..1723622 100644
> >> --- a/monitor.h
> >> +++ b/monitor.h
> >> @@ -86,4 +86,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict,
> >> QObject **ret);
> >>
> >> int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
> >>
> >> +int get_key_index(const char *key);
> >> +
> >> #endif /* !MONITOR_H */
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index 3b6e346..5717467 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -1862,3 +1862,48 @@
> >> # Since: 0.14.0
> >> ##
> >> { 'command': 'netdev_del', 'data': {'id': 'str'} }
> >> +
> >> +##
> >> +# @KeyCodes:
> >> +#
> >> +# An enumeration of key name.
> >> +#
> >> +# This is used by the sendkey command.
> >> +#
> >> +# Since: 1.2
> >> +##
> >> +{ 'enum': 'KeyCodes',
> >> + 'data': [ 'shift', 'shift_r', 'alt', 'alt_r', 'altgr', 'altgr_r',
> >> 'ctrl',
> >> + 'ctrl_r', 'menu', 'esc', '1', '2', '3', '4', '5', '6', '7',
> >> '8',
> >> + '9', '0', 'minus', 'equal', 'backspace', 'tab', 'q', 'w', 'e',
> >> + 'r', 't', 'y', 'u', 'i', 'o', 'p', 'bracket_left',
> >> 'bracket_right',
> >> + 'ret', 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l',
> >> 'semicolon',
> >> + 'apostrophe', 'grave_accent', 'backslash', 'z', 'x', 'c',
> >> 'v', 'b',
> >> + 'n', 'm', 'comma', 'dot', 'slash', 'asterisk', 'spc',
> >> 'caps_lock',
> >> + 'f1', 'f2', 'f3', 'f4', 'f5', 'f6', 'f7', 'f8', 'f9', 'f10',
> >> + 'num_lock', 'scroll_lock', 'kp_divide', 'kp_multiply',
> >> + 'kp_subtract', 'kp_add', 'kp_enter', 'kp_decimal', 'sysrq',
> >> 'kp_0',
> >> + 'kp_1', 'kp_2', 'kp_3', 'kp_4', 'kp_5', 'kp_6', 'kp_7',
> >> 'kp_8',
> >> + 'kp_9', 'less', 'f11', 'f12', 'print', 'home', 'pgup',
> >> 'pgdn', 'end',
> >> + 'left', 'up', 'down', 'right', 'insert', 'delete', 'stop',
> >> 'again',
> >> + 'props', 'undo', 'front', 'copy', 'open', 'paste', 'find',
> >> 'cut',
> >> + 'lf', 'help', 'meta_l', 'meta_r', 'compose' ] }
> >> +
> >> +##
> >> +# @sendkey:
> >> +#
> >> +# Send keys to VM.
> >> +#
> >> +# @keys: key sequence
> >
> >> +# @hold-time: time to delay key up events, milliseconds
> >
> > Please, say that hold-time is optional (look in this file for examples) and
> > mention its default value.
>
> Got it.
>
> >> +#
> >> +# Returns: Nothing on success
> >> +# If key is unknown or redundant, QERR_INVALID_PARAMETER
> >
> > How a key is redundant? And please do
> > s/QERR_INVALID_PARAMETER/InvalidParameter
>
>
> 16 limitation had been removed, the 'redundant' also needs to be removed.
>
> >> +#
> >> +# Notes: Send keys to the guest. 'keys' could be the name of the
> >> +# key. Use a JSON array to press several keys simultaneously.
> >
> > This should be part of the command's description, not a bottom note.
>
>
> >> +#
> >> +# Since: 1.2
> >> +##
> >> +{ 'command': 'sendkey',
> >> + 'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index 2e1a38e..bcc6c4b 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -335,6 +335,33 @@ Example:
> >> EQMP
> >>
> >> {
> >> + .name = "sendkey",
> >> + .args_type = "keys:O,hold-time:i?",
> >> + .mhandler.cmd_new = qmp_marshal_input_sendkey,
> >> + },
> >> +
> >> +SQMP
> >> +sendkey
> >> +----------
> >> +
> >> +Send keys to VM.
> >> +
> >> +Arguments:
> >> +
> >> +keys array:
> >> + - "key": key sequence (json-string)
> >> +
> >> +- hold-time: time to delay key up events, milliseconds (josn-int,
> >> optional)
> >> +
> >> +Example:
> >> +
> >> +-> { "execute": "sendkey",
> >> + "arguments": { 'keys': [ 'ctrl', 'alt', 'delete' ] } }
> >> +<- { "return": {} }
> >> +
> >> +EQMP
> >> +
> >> + {
> >> .name = "cpu",
> >> .args_type = "index:i",
> >> .mhandler.cmd_new = qmp_marshal_input_cpu,
>
> Thanks for your time.
>
