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

[Michael Goldish <mgoldish@redhat.com>]

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 } }"
>>  
> 
>