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

[Luiz Capitulino <lcapitulino@redhat.com>]

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?