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

[Daniel P. Berrange]

On Wed, Jul 21, 2010 at 03:44:14PM -0300, Luiz Capitulino wrote:
> On Sun, 18 Jul 2010 15:43:55 +0300
> Michael Goldish <address@hidden> wrote:
> 
> > Signed-off-by: Michael Goldish <address@hidden>
> 
> 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
-- 
|: 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 :|