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

[Luiz Capitulino]

On Wed, 21 Jul 2010 23:30:56 +0300
Michael Goldish <address@hidden> wrote:

> On 07/21/2010 09:44 PM, 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.
> 
> 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 } }"
> >>  
> > 
> > 
>