Patchwork Monitor: Convert do_sendkey() to QObject, QError

login
register
mail settings
Submitter Michael Goldish
Date July 18, 2010, 12:43 p.m.
Message ID <1279457035-28657-1-git-send-email-mgoldish@redhat.com>
Download mbox | patch
Permalink /patch/59167/
State New
Headers show

Comments

Michael Goldish - July 18, 2010, 12:43 p.m.
Signed-off-by: Michael Goldish <mgoldish@redhat.com>
---
 monitor.c       |   15 ++++++++-------
 qemu-monitor.hx |   22 +++++++++++++++++++++-
 qerror.c        |   12 ++++++++++++
 qerror.h        |    9 +++++++++
 4 files changed, 50 insertions(+), 8 deletions(-)
Luiz Capitulino - July 21, 2010, 6:44 p.m.
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 } }"
>
Daniel P. Berrange - July 21, 2010, 7:06 p.m.
On Wed, Jul 21, 2010 at 03:44:14PM -0300, Luiz Capitulino wrote:
> 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.

I can see why keynames are nice in the text monitor, but is there a need
for JSON mode to accept keynames too ? If we're focusing on providing a
machine protocol, then keycodes seem like they sufficient to me. Apps can
using the command can provide symbol constants for the keycodes, or string
if actually needed by their end users

> 
> 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)

Having 'hold-time' which applies to the full list of keys is limiting
the flexibility of apps. eg, it means you can only do

   down ctrl
   down alt
   down f1
   wait 100ms
   up ctrl
   up alt
   up f1

Again I can see why the impl works this way currently, because it is
clearly a nicer option for humans. For a machine protocol though it
seems sub-optimal. What if app needed more flexibility over ordering
of press+release events eg to release in a different order

   down ctrl
   down alt
   down f1
   wait 100ms
   up f1
   up ctrl
   up alt

Should we just follow VNC and explicitly have a up/down flag in
the protocol & let press & release events be sent separately.

  { "execute": "sendkey", "arguments":  { "keycode": 0x31, "down": true } }

We could allow multiple keycodes in one message

  { "execute": "sendkey", "arguments":  { "keycodes": [ 0x31, 0x32 ], "down": true } }

but its not really adding critical functionality that can't be got by
sending a sequence of sendkey commands in a row.

Daniel
Michael Goldish - July 21, 2010, 8:30 p.m.
On 07/21/2010 09:44 PM, Luiz Capitulino wrote:
> 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.

No, it's not urgent.

>> ---
>>  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.

What do you mean by "add a new args_type"?  Do you mean I should add a
new type 'a', similar to 's' and 'i', that represents a dash separated
string/int array?  If so, I suppose obtaining the array in do_sendkey()
would involve something like

QList *keys = qdict_get_qlist(qdict, "keys");

Is this correct?  I'm just asking to be sure.

> 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.

What's an async handler?  Feel free to point me to some documentation if
there is any.

>> +- "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.

OK.  Thanks for the comments.

>> @@ -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 } }"
>>  
> 
>
Luiz Capitulino - July 22, 2010, 1:28 p.m.
On Wed, 21 Jul 2010 20:06:56 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Jul 21, 2010 at 03:44:14PM -0300, Luiz Capitulino wrote:
> > 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.
> 
> I can see why keynames are nice in the text monitor, but is there a need
> for JSON mode to accept keynames too ? If we're focusing on providing a
> machine protocol, then keycodes seem like they sufficient to me. Apps can
> using the command can provide symbol constants for the keycodes, or string
> if actually needed by their end users

Right. That should be a user monitor's feature, not QMP's.

> > 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)
> 
> Having 'hold-time' which applies to the full list of keys is limiting
> the flexibility of apps. eg, it means you can only do
> 
>    down ctrl
>    down alt
>    down f1
>    wait 100ms
>    up ctrl
>    up alt
>    up f1
> 
> Again I can see why the impl works this way currently, because it is
> clearly a nicer option for humans. For a machine protocol though it
> seems sub-optimal. What if app needed more flexibility over ordering
> of press+release events eg to release in a different order
> 
>    down ctrl
>    down alt
>    down f1
>    wait 100ms
>    up f1
>    up ctrl
>    up alt
> 
> Should we just follow VNC and explicitly have a up/down flag in
> the protocol & let press & release events be sent separately.
> 
>   { "execute": "sendkey", "arguments":  { "keycode": 0x31, "down": true } }
> 
> We could allow multiple keycodes in one message
> 
>   { "execute": "sendkey", "arguments":  { "keycodes": [ 0x31, 0x32 ], "down": true } }
> 
> but its not really adding critical functionality that can't be got by
> sending a sequence of sendkey commands in a row.

Hm, looks good to me, but then the hold time would be the time period
between the down/up commands. This won't be reliable in case the client
wants to exactly wait 100ms, as we can have network latency, for example.

Isn't this a problem? I believe VNC doesn't have this feature, right?
Luiz Capitulino - July 22, 2010, 1:39 p.m.
On Wed, 21 Jul 2010 23:30:56 +0300
Michael Goldish <mgoldish@redhat.com> wrote:

> On 07/21/2010 09:44 PM, Luiz Capitulino wrote:
> > 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.
> 
> No, it's not urgent.
> 
> >> ---
> >>  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.
> 
> What do you mean by "add a new args_type"?  Do you mean I should add a
> new type 'a', similar to 's' and 'i', that represents a dash separated
> string/int array?  If so, I suppose obtaining the array in do_sendkey()
> would involve something like
> 
> QList *keys = qdict_get_qlist(qdict, "keys");
> 
> Is this correct?  I'm just asking to be sure.

It's exactly that. The user monitor does the fancy user parsing, handlers
handle QMP data only.

But note that the recommended QMP development process is to send the
documentation patch first, as an RFC. This way we can discuss and stabilize
the interface before touching the code (which is easier and avoids the
code dictating the interface).

So, it's a good idea to rewrite the documentation based on my discussion
with Daniel, and re-send it.

This process is not documented yet, will send a patch shortly.

> > 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.
> 
> What's an async handler?  Feel free to point me to some documentation if
> there is any.

  http://wiki.qemu.org/Features/QMP_0.14

It's quite beyond this conversion, you don't have to worry about that.

> 
> >> +- "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.
> 
> OK.  Thanks for the comments.
> 
> >> @@ -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 } }"
> >>  
> > 
> > 
>
Daniel P. Berrange - July 22, 2010, 1:45 p.m.
On Thu, Jul 22, 2010 at 10:28:39AM -0300, Luiz Capitulino wrote:
> On Wed, 21 Jul 2010 20:06:56 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Wed, Jul 21, 2010 at 03:44:14PM -0300, Luiz Capitulino wrote:
> > > 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)
> > 
> > Having 'hold-time' which applies to the full list of keys is limiting
> > the flexibility of apps. eg, it means you can only do
> > 
> >    down ctrl
> >    down alt
> >    down f1
> >    wait 100ms
> >    up ctrl
> >    up alt
> >    up f1
> > 
> > Again I can see why the impl works this way currently, because it is
> > clearly a nicer option for humans. For a machine protocol though it
> > seems sub-optimal. What if app needed more flexibility over ordering
> > of press+release events eg to release in a different order
> > 
> >    down ctrl
> >    down alt
> >    down f1
> >    wait 100ms
> >    up f1
> >    up ctrl
> >    up alt
> > 
> > Should we just follow VNC and explicitly have a up/down flag in
> > the protocol & let press & release events be sent separately.
> > 
> >   { "execute": "sendkey", "arguments":  { "keycode": 0x31, "down": true } }
> > 
> > We could allow multiple keycodes in one message
> > 
> >   { "execute": "sendkey", "arguments":  { "keycodes": [ 0x31, 0x32 ], "down": true } }
> > 
> > but its not really adding critical functionality that can't be got by
> > sending a sequence of sendkey commands in a row.
> 
> Hm, looks good to me, but then the hold time would be the time period
> between the down/up commands. This won't be reliable in case the client
> wants to exactly wait 100ms, as we can have network latency, for example.
> 
> Isn't this a problem? I believe VNC doesn't have this feature, right?

Correct, VNC just sends each individual press / release event as a separate
message, so you can have network delay effects there too.

If we needed to support precise delays safe from network delay for some
simulation needs, then you'd probably need a more complex structure
where you can provide a whole sequence of operations. And why stop at
keys, including mouse movement & buttons to.

   { "execute": "sendinput", "arguments":  { 
       "sequence" : [
          { "event": "keypress", "keycode": 0x31 },
          { "event": "keypress", "keycode": 0x75 },
          { "event": "wait", "delay": 100 },
          { "event": "mousepress", "button": 1 },
          { "event": "mousemove", "xdelta": 1, "ydelta": 1 },
          { "event": "keyrelease", "keycode": 0x31 },
          { "event": "wait", "delay": 100 },
          { "event": "keyrelease", "keycode": 0x75 },
          { "event": "mousepos", "x": 102, "y": 102 },
       ] 
   } }

This is getting kind of advanced now. Whether we need this vs the simpler
sendkey, mouse_move, etc command comes down to whether we need ability to
set precise delays between events.  We could stick with the individual
simple commands & add a advanced one alter  

Daniel
--
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Luiz Capitulino - July 22, 2010, 1:50 p.m.
On Thu, 22 Jul 2010 14:45:35 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Thu, Jul 22, 2010 at 10:28:39AM -0300, Luiz Capitulino wrote:
> > On Wed, 21 Jul 2010 20:06:56 +0100
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > 
> > > On Wed, Jul 21, 2010 at 03:44:14PM -0300, Luiz Capitulino wrote:
> > > > 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)
> > > 
> > > Having 'hold-time' which applies to the full list of keys is limiting
> > > the flexibility of apps. eg, it means you can only do
> > > 
> > >    down ctrl
> > >    down alt
> > >    down f1
> > >    wait 100ms
> > >    up ctrl
> > >    up alt
> > >    up f1
> > > 
> > > Again I can see why the impl works this way currently, because it is
> > > clearly a nicer option for humans. For a machine protocol though it
> > > seems sub-optimal. What if app needed more flexibility over ordering
> > > of press+release events eg to release in a different order
> > > 
> > >    down ctrl
> > >    down alt
> > >    down f1
> > >    wait 100ms
> > >    up f1
> > >    up ctrl
> > >    up alt
> > > 
> > > Should we just follow VNC and explicitly have a up/down flag in
> > > the protocol & let press & release events be sent separately.
> > > 
> > >   { "execute": "sendkey", "arguments":  { "keycode": 0x31, "down": true } }
> > > 
> > > We could allow multiple keycodes in one message
> > > 
> > >   { "execute": "sendkey", "arguments":  { "keycodes": [ 0x31, 0x32 ], "down": true } }
> > > 
> > > but its not really adding critical functionality that can't be got by
> > > sending a sequence of sendkey commands in a row.
> > 
> > Hm, looks good to me, but then the hold time would be the time period
> > between the down/up commands. This won't be reliable in case the client
> > wants to exactly wait 100ms, as we can have network latency, for example.
> > 
> > Isn't this a problem? I believe VNC doesn't have this feature, right?
> 
> Correct, VNC just sends each individual press / release event as a separate
> message, so you can have network delay effects there too.
> 
> If we needed to support precise delays safe from network delay for some
> simulation needs, then you'd probably need a more complex structure
> where you can provide a whole sequence of operations. And why stop at
> keys, including mouse movement & buttons to.
> 
>    { "execute": "sendinput", "arguments":  { 
>        "sequence" : [
>           { "event": "keypress", "keycode": 0x31 },
>           { "event": "keypress", "keycode": 0x75 },
>           { "event": "wait", "delay": 100 },
>           { "event": "mousepress", "button": 1 },
>           { "event": "mousemove", "xdelta": 1, "ydelta": 1 },
>           { "event": "keyrelease", "keycode": 0x31 },
>           { "event": "wait", "delay": 100 },
>           { "event": "keyrelease", "keycode": 0x75 },
>           { "event": "mousepos", "x": 102, "y": 102 },
>        ] 
>    } }
> 
> This is getting kind of advanced now. Whether we need this vs the simpler
> sendkey, mouse_move, etc command comes down to whether we need ability to
> set precise delays between events.  We could stick with the individual
> simple commands & add a advanced one alter  

Yeah, agreed and I think the current design (ie. hold_time) is simpler.
Daniel P. Berrange - July 22, 2010, 2:03 p.m.
On Thu, Jul 22, 2010 at 10:50:00AM -0300, Luiz Capitulino wrote:
> On Thu, 22 Jul 2010 14:45:35 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Thu, Jul 22, 2010 at 10:28:39AM -0300, Luiz Capitulino wrote:
> > > On Wed, 21 Jul 2010 20:06:56 +0100
> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > 
> > > > On Wed, Jul 21, 2010 at 03:44:14PM -0300, Luiz Capitulino wrote:
> > > > > 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)
> > > > 
> > > > Having 'hold-time' which applies to the full list of keys is limiting
> > > > the flexibility of apps. eg, it means you can only do
> > > > 
> > > >    down ctrl
> > > >    down alt
> > > >    down f1
> > > >    wait 100ms
> > > >    up ctrl
> > > >    up alt
> > > >    up f1
> > > > 
> > > > Again I can see why the impl works this way currently, because it is
> > > > clearly a nicer option for humans. For a machine protocol though it
> > > > seems sub-optimal. What if app needed more flexibility over ordering
> > > > of press+release events eg to release in a different order
> > > > 
> > > >    down ctrl
> > > >    down alt
> > > >    down f1
> > > >    wait 100ms
> > > >    up f1
> > > >    up ctrl
> > > >    up alt
> > > > 
> > > > Should we just follow VNC and explicitly have a up/down flag in
> > > > the protocol & let press & release events be sent separately.
> > > > 
> > > >   { "execute": "sendkey", "arguments":  { "keycode": 0x31, "down": true } }
> > > > 
> > > > We could allow multiple keycodes in one message
> > > > 
> > > >   { "execute": "sendkey", "arguments":  { "keycodes": [ 0x31, 0x32 ], "down": true } }
> > > > 
> > > > but its not really adding critical functionality that can't be got by
> > > > sending a sequence of sendkey commands in a row.
> > > 
> > > Hm, looks good to me, but then the hold time would be the time period
> > > between the down/up commands. This won't be reliable in case the client
> > > wants to exactly wait 100ms, as we can have network latency, for example.
> > > 
> > > Isn't this a problem? I believe VNC doesn't have this feature, right?
> > 
> > Correct, VNC just sends each individual press / release event as a separate
> > message, so you can have network delay effects there too.
> > 
> > If we needed to support precise delays safe from network delay for some
> > simulation needs, then you'd probably need a more complex structure
> > where you can provide a whole sequence of operations. And why stop at
> > keys, including mouse movement & buttons to.
> > 
> >    { "execute": "sendinput", "arguments":  { 
> >        "sequence" : [
> >           { "event": "keypress", "keycode": 0x31 },
> >           { "event": "keypress", "keycode": 0x75 },
> >           { "event": "wait", "delay": 100 },
> >           { "event": "mousepress", "button": 1 },
> >           { "event": "mousemove", "xdelta": 1, "ydelta": 1 },
> >           { "event": "keyrelease", "keycode": 0x31 },
> >           { "event": "wait", "delay": 100 },
> >           { "event": "keyrelease", "keycode": 0x75 },
> >           { "event": "mousepos", "x": 102, "y": 102 },
> >        ] 
> >    } }
> > 
> > This is getting kind of advanced now. Whether we need this vs the simpler
> > sendkey, mouse_move, etc command comes down to whether we need ability to
> > set precise delays between events.  We could stick with the individual
> > simple commands & add a advanced one alter  
> 
> Yeah, agreed and I think the current design (ie. hold_time) is simpler.

When I said stick with simpler sendkey, I mean the style I outlined
without any hold time at all

> > > >   { "execute": "sendkey", "arguments":  { "keycode": 0x31, "down": true } }

If its good enough for VNC & SPICE, it should be good enough for most
monitor users, even more so since QMP communication is usually over a
low latency UNIX domain socket. I think hold time is a flawed concept
as currently provide since it prevents up/down interleaving by sending
a sequence of QMP commands. 

Daniel
Luiz Capitulino - July 22, 2010, 2:36 p.m.
On Thu, 22 Jul 2010 15:03:27 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Thu, Jul 22, 2010 at 10:50:00AM -0300, Luiz Capitulino wrote:
> > On Thu, 22 Jul 2010 14:45:35 +0100
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > 
> > > On Thu, Jul 22, 2010 at 10:28:39AM -0300, Luiz Capitulino wrote:
> > > > On Wed, 21 Jul 2010 20:06:56 +0100
> > > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > > 
> > > > > On Wed, Jul 21, 2010 at 03:44:14PM -0300, Luiz Capitulino wrote:
> > > > > > 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)
> > > > > 
> > > > > Having 'hold-time' which applies to the full list of keys is limiting
> > > > > the flexibility of apps. eg, it means you can only do
> > > > > 
> > > > >    down ctrl
> > > > >    down alt
> > > > >    down f1
> > > > >    wait 100ms
> > > > >    up ctrl
> > > > >    up alt
> > > > >    up f1
> > > > > 
> > > > > Again I can see why the impl works this way currently, because it is
> > > > > clearly a nicer option for humans. For a machine protocol though it
> > > > > seems sub-optimal. What if app needed more flexibility over ordering
> > > > > of press+release events eg to release in a different order
> > > > > 
> > > > >    down ctrl
> > > > >    down alt
> > > > >    down f1
> > > > >    wait 100ms
> > > > >    up f1
> > > > >    up ctrl
> > > > >    up alt
> > > > > 
> > > > > Should we just follow VNC and explicitly have a up/down flag in
> > > > > the protocol & let press & release events be sent separately.
> > > > > 
> > > > >   { "execute": "sendkey", "arguments":  { "keycode": 0x31, "down": true } }
> > > > > 
> > > > > We could allow multiple keycodes in one message
> > > > > 
> > > > >   { "execute": "sendkey", "arguments":  { "keycodes": [ 0x31, 0x32 ], "down": true } }
> > > > > 
> > > > > but its not really adding critical functionality that can't be got by
> > > > > sending a sequence of sendkey commands in a row.
> > > > 
> > > > Hm, looks good to me, but then the hold time would be the time period
> > > > between the down/up commands. This won't be reliable in case the client
> > > > wants to exactly wait 100ms, as we can have network latency, for example.
> > > > 
> > > > Isn't this a problem? I believe VNC doesn't have this feature, right?
> > > 
> > > Correct, VNC just sends each individual press / release event as a separate
> > > message, so you can have network delay effects there too.
> > > 
> > > If we needed to support precise delays safe from network delay for some
> > > simulation needs, then you'd probably need a more complex structure
> > > where you can provide a whole sequence of operations. And why stop at
> > > keys, including mouse movement & buttons to.
> > > 
> > >    { "execute": "sendinput", "arguments":  { 
> > >        "sequence" : [
> > >           { "event": "keypress", "keycode": 0x31 },
> > >           { "event": "keypress", "keycode": 0x75 },
> > >           { "event": "wait", "delay": 100 },
> > >           { "event": "mousepress", "button": 1 },
> > >           { "event": "mousemove", "xdelta": 1, "ydelta": 1 },
> > >           { "event": "keyrelease", "keycode": 0x31 },
> > >           { "event": "wait", "delay": 100 },
> > >           { "event": "keyrelease", "keycode": 0x75 },
> > >           { "event": "mousepos", "x": 102, "y": 102 },
> > >        ] 
> > >    } }
> > > 
> > > This is getting kind of advanced now. Whether we need this vs the simpler
> > > sendkey, mouse_move, etc command comes down to whether we need ability to
> > > set precise delays between events.  We could stick with the individual
> > > simple commands & add a advanced one alter  
> > 
> > Yeah, agreed and I think the current design (ie. hold_time) is simpler.
> 
> When I said stick with simpler sendkey, I mean the style I outlined
> without any hold time at all
> 
> > > > >   { "execute": "sendkey", "arguments":  { "keycode": 0x31, "down": true } }
> 
> If its good enough for VNC & SPICE, it should be good enough for most
> monitor users, even more so since QMP communication is usually over a
> low latency UNIX domain socket. I think hold time is a flawed concept
> as currently provide since it prevents up/down interleaving by sending
> a sequence of QMP commands. 

I don't want to suck the QMP interface because of that, but the problem
here is how to maintain the user monitor's current behavior.

The best option is to wait for the user monitor/QMP split. This way the
hold time could be a feature of the user monitor command only.

Or, we could add a new command. But this would just be a workaround..
Artyom Tarasenko - July 22, 2010, 9:17 p.m.
2010/7/21 Luiz Capitulino <lcapitulino@redhat.com>:

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

Was it announced somewhere?
Luiz Capitulino - July 22, 2010, 10:27 p.m.
On Thu, 22 Jul 2010 23:17:39 +0200
Artyom Tarasenko <atar4qemu@googlemail.com> wrote:

> 2010/7/21 Luiz Capitulino <lcapitulino@redhat.com>:
> 
> > Do you need this for 0.13? I think the development window is already closed.
> 
> Was it announced somewhere?

I heard it on the conf call and iirc it was said on some thread too, but I
think it will only be official when we get the first -rc.

Patch

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)
+- "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
@@ -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 } }"