Re: [Qemu-devel] [PATCH 3/3] qapi: convert sendkey

[Luiz Capitulino]

On Fri, 25 May 2012 11:32:01 +0800
Amos Kong <address@hidden> 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 <address@hidden>
> ---
>  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,