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

Michael Goldish
2010-07-21T20:30:40Z

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