Re: [Qemu-devel] [PATCH] Monitor: Convert do_sendkey() to QObject, QError

[Luiz Capitulino <lcapitulino@redhat.com>]

On Sun, 18 Jul 2010 15:43:55 +0300
Michael Goldish <mgoldish@redhat.com> wrote:

> Signed-off-by: Michael Goldish <mgoldish@redhat.com>

Do you need this for 0.13? I think the development window is already closed.

> ---
>  monitor.c       |   15 ++++++++-------
>  qemu-monitor.hx |   22 +++++++++++++++++++++-
>  qerror.c        |   12 ++++++++++++
>  qerror.h        |    9 +++++++++
>  4 files changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d5377de..082c29d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1614,7 +1614,7 @@ static void release_keys(void *opaque)
>      }
>  }
>  
> -static void do_sendkey(Monitor *mon, const QDict *qdict)
> +static int do_sendkey(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>      char keyname_buf[16];
>      char *separator;
> @@ -1636,18 +1636,18 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
>          if (keyname_len > 0) {
>              pstrcpy(keyname_buf, sizeof(keyname_buf), string);
>              if (keyname_len > sizeof(keyname_buf) - 1) {
> -                monitor_printf(mon, "invalid key: '%s...'\n", keyname_buf);
> -                return;
> +                qerror_report(QERR_INVALID_KEY, keyname_buf);
> +                return -1;
>              }
>              if (i == MAX_KEYCODES) {
> -                monitor_printf(mon, "too many keys\n");
> -                return;
> +                qerror_report(QERR_TOO_MANY_KEYS);
> +                return -1;
>              }
>              keyname_buf[keyname_len] = 0;
>              keycode = get_keycode(keyname_buf);
>              if (keycode < 0) {
> -                monitor_printf(mon, "unknown key: '%s'\n", keyname_buf);
> -                return;
> +                qerror_report(QERR_UNKNOWN_KEY, keyname_buf);
> +                return -1;
>              }
>              keycodes[i++] = keycode;
>          }
> @@ -1666,6 +1666,7 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
>      /* delayed key up events */
>      qemu_mod_timer(key_timer, qemu_get_clock(vm_clock) +
>                     muldiv64(get_ticks_per_sec(), hold_time, 1000));
> +    return 0;
>  }
>  
>  static int mouse_button_state;
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index f7e46c4..8b6cf09 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -532,7 +532,8 @@ ETEXI
>          .args_type  = "string: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,
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_sendkey,
>      },
>  
>  STEXI
> @@ -549,6 +550,25 @@ sendkey ctrl-alt-f1
>  This command is useful to send keys that your graphical user interface
>  intercepts at low level, such as @code{ctrl-alt-f1} in X Window.
>  ETEXI
> +SQMP
> +sendkey
> +-------
> +
> +Send keys to the emulator.
> +
> +Arguments:
> +
> +- "string": key names or key codes in hexadecimal format separated by dashes (json-string)

This is leaking bad stuff from the user monitor into QMP.

I think we should have a "keys" key, which accepts an array of keys. Strings
are key names, integers are key codes. Unfortunately, key codes will have to
be specified in decimal.

Example:

 { "execute": "sendkey", "arguments": { "keys": ["foo", 10, "bar", 15] } }

However, in order to maintain the current user monitor behavior, you will
have to add a new args_type so that you can move the current parsing out
of the handler to the user monitor parser. Then you'll have to change the
handler to work with an array.

A bit of work.

Another related issue is that, this probably should an async handler. But
as we don't have the proper infrastructure yet, I'm ok with having this in
its current form.

> +- "hold_time": duration in milliseconds to hold the keys down (json-int, optional, default=100)
> +
> +Example:
> +
> +-> { "execute": "sendkey", "arguments": { "string": "ctrl-alt-f1" } }
> +<- { "return": {} }
> +
> +Note: if several keys are given they are pressed simultaneously.
> +
> +EQMP
>  
>      {
>          .name       = "system_reset",
> diff --git a/qerror.c b/qerror.c
> index 44d0bf8..34a698e 100644
> --- a/qerror.c
> +++ b/qerror.c

Please, split this into two patches: the errors first, then the conversion.
That helps reviewing and reverting.

> @@ -117,6 +117,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Invalid block format '%(name)'",
>      },
>      {
> +        .error_fmt = QERR_INVALID_KEY,
> +        .desc      = "Invalid key '%(key)...'",
> +    },
> +    {
>          .error_fmt = QERR_INVALID_PARAMETER,
>          .desc      = "Invalid parameter '%(name)'",
>      },
> @@ -185,10 +189,18 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Too many open files",
>      },
>      {
> +        .error_fmt = QERR_TOO_MANY_KEYS,
> +        .desc      = "Too many keys",
> +    },
> +    {
>          .error_fmt = QERR_UNDEFINED_ERROR,
>          .desc      = "An undefined error has ocurred",
>      },
>      {
> +        .error_fmt = QERR_UNKNOWN_KEY,
> +        .desc      = "Unknown key '%(key)'",
> +    },
> +    {
>          .error_fmt = QERR_VNC_SERVER_FAILED,
>          .desc      = "Could not start VNC server on %(target)",
>      },
> diff --git a/qerror.h b/qerror.h
> index 77ae574..a23630c 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -103,6 +103,9 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_INVALID_BLOCK_FORMAT \
>      "{ 'class': 'InvalidBlockFormat', 'data': { 'name': %s } }"
>  
> +#define QERR_INVALID_KEY \
> +    "{ 'class': 'InvalidKey', 'data': { 'key': %s } }"
> +
>  #define QERR_INVALID_PARAMETER \
>      "{ 'class': 'InvalidParameter', 'data': { 'name': %s } }"
>  
> @@ -154,9 +157,15 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_TOO_MANY_FILES \
>      "{ 'class': 'TooManyFiles', 'data': {} }"
>  
> +#define QERR_TOO_MANY_KEYS \
> +    "{ 'class': 'TooManyKeys', 'data': {} }"
> +
>  #define QERR_UNDEFINED_ERROR \
>      "{ 'class': 'UndefinedError', 'data': {} }"
>  
> +#define QERR_UNKNOWN_KEY \
> +    "{ 'class': 'UnknownKey', 'data': { 'key': %s } }"
> +
>  #define QERR_VNC_SERVER_FAILED \
>      "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
>