On Wed, 21 Jul 2010 20:06:56 +0100
"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> >
> > > Signed-off-by: Michael Goldish <mgoldish@xxxxxxxxxx>
> >
> > 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?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
