On Fri, 25 May 2012 11:32:01 +0800
Amos Kong <ak...@redhat.com> wrote:
> Convert 'sendkey' to use. do_sendkey() depends on some variables
> in monitor.c, so reserve qmp_sendkey() to monitor.c
> Rename 'string' to 'keys', rename 'hold_time' to 'hold-time'
Splitting the args rename to a different patch would make review easier. Also,
please, include a cover letter as patch 0/3.
In general looks good, but apart the argument re-work others suggested I have
some comments below.
> Signed-off-by: Amos Kong <ak...@redhat.com>
> ---
> hmp-commands.hx | 4 ++--
> hmp.c | 11 +++++++++++
> hmp.h | 1 +
> monitor.c | 21 ++++++++++-----------
> qapi-schema.json | 20 ++++++++++++++++++++
> qmp-commands.hx | 24 ++++++++++++++++++++++++
> 6 files changed, 68 insertions(+), 13 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index af18156..afbfa61 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -502,10 +502,10 @@ ETEXI
>
> {
> .name = "sendkey",
> - .args_type = "string:s,hold_time:i?",
> + .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 bb0952e..abb7b59 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -947,3 +947,14 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
> qmp_device_del(id, &err);
> hmp_handle_error(mon, &err);
> }
> +
> +void hmp_sendkey(Monitor *mon, const QDict *qdict)
> +{
> + 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);
> + Error *err = NULL;
> +
> + qmp_sendkey(keys, has_hold_time, hold_time, &err);
> + hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 443b812..40de72c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -61,5 +61,6 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict
> *qdict);
> void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> void hmp_migrate(Monitor *mon, const QDict *qdict);
> void hmp_device_del(Monitor *mon, const QDict *qdict);
> +void hmp_sendkey(Monitor *mon, const QDict *qdict);
>
> #endif
> diff --git a/monitor.c b/monitor.c
> index 12a6fe2..238f8da 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1377,14 +1377,12 @@ static void release_keys(void *opaque)
> }
> }
>
> -static void do_sendkey(Monitor *mon, const QDict *qdict)
> +void qmp_sendkey(const char *keys, bool has_hold_time, int64_t hold_time,
> + Error **err)
> {
> char keyname_buf[16];
> char *separator;
> int keyname_len, keycode, i;
> - const char *string = qdict_get_str(qdict, "string");
> - int has_hold_time = qdict_haskey(qdict, "hold_time");
> - int hold_time = qdict_get_try_int(qdict, "hold_time", -1);
>
> if (nb_pending_keycodes > 0) {
> qemu_del_timer(key_timer);
> @@ -1394,29 +1392,30 @@ static void do_sendkey(Monitor *mon, const QDict
> *qdict)
> hold_time = 100;
> i = 0;
> while (1) {
> - separator = strchr(string, '-');
> - keyname_len = separator ? separator - string : strlen(string);
> + separator = strchr(keys, '-');
> + keyname_len = separator ? separator - keys : strlen(keys);
> if (keyname_len > 0) {
> - pstrcpy(keyname_buf, sizeof(keyname_buf), string);
> + pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
> if (keyname_len > sizeof(keyname_buf) - 1) {
> - monitor_printf(mon, "invalid key: '%s...'\n", keyname_buf);
> + error_set(err, QERR_INVALID_PARAMETER_VALUE, "keyname_buf",
> + keyname_buf);
You should pass "key name" in the first string.
> return;
> }
> if (i == MAX_KEYCODES) {
> - monitor_printf(mon, "too many keys\n");
> + error_set(err, QERR_TOO_MANY_PARAMETERS);
> return;
> }
I know I suggested TOO_MANY_PARAMETERS myself, but having QERR_OVERFLOW
would be better (and we should of course document that in the schema).
On the other hand I wonder if we should limit this, do we limit filenames
for example? Or maybe we should limit it to a bigger size, like 256 bytes.
Your choice.
> keyname_buf[keyname_len] = 0;
> keycode = get_keycode(keyname_buf);
> if (keycode < 0) {
> - monitor_printf(mon, "unknown key: '%s'\n", keyname_buf);
> + error_set(err, QERR_INVALID_PARAMETER, keyname_buf);
> return;
> }
> keycodes[i++] = keycode;
> }
> if (!separator)
> break;
> - string = separator + 1;
> + keys = separator + 1;
> }
> nb_pending_keycodes = i;
> /* key down events */
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 2ca7195..d1799bc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1755,3 +1755,23 @@
> # Since: 0.14.0
> ##
> { 'command': 'device_del', 'data': {'id': 'str'} }
> +
> +##
> +# @sendkey:
> +#
> +# Send keys to VM.
> +#
> +# @keys: key sequence
> +# @hold-time: time to delay key up events
Miliseconds?
> +#
> +# Returns: Nothing on success
> +# If key is unknown or redundant, QERR_INVALID_PARAMETER
> +# If key is invalid, QERR_INVALID_PARAMETER_VALUE
Please, use the class name (eg. InvalidParameter) instead of the qerror macro.
Also, don't forget to document the Overflow error.
> +#
> +# Notes: Send @var{keys} to the emulator. @var{keys} could be the name of the
> +# key or the raw value in either decimal or hexadecimal format. Use
> +# @code{-} to press several keys simultaneously.
Please, move this right below "Send keys to the VM" and drop the @var{}
notation,
as it's only used in the .hx files.
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'sendkey', 'data': {'keys': 'str', '*hold-time': 'int'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index db980fa..ad6fc21 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -335,6 +335,30 @@ Example:
> EQMP
>
> {
> + .name = "sendkey",
> + .args_type = "keys:s,hold-time:i?",
> + .mhandler.cmd_new = qmp_marshal_input_sendkey,
> + },
> +
> +SQMP
> +sendkey
> +----------
> +
> +Send keys to VM.
> +
> +Arguments:
> +
> +- "keys": key sequence (json-string)
> +- "hold-time": time to delay key up events (josn-string, optional)
> +
> +Example:
> +
> +-> { "execute": "sendkey", "arguments": { "keys": "ctrl-alt-delete",
> "hold-time": 200 } }
> +<- { "return": {} }
> +
> +EQMP
> +
> + {
> .name = "cpu",
> .args_type = "index:i",
> .mhandler.cmd_new = qmp_marshal_input_cpu,
