[Qemu-devel] [PATCH v2 0/6] convert sendkey to qapi

[Qemu-devel] [PATCH v2 0/6] convert sendkey to qapi

[Amos Kong]

This series converted 'sendkey' command to qapi.

Amos Kong (6):
  qerror: add QERR_OVERFLOW
  fix doc of using raw values with sendkey
  rename keyname '<' to 'less'
  hmp: rename arguments
  qapi: generate list struct and visit_list for enum
  qapi: convert sendkey

 hmp-commands.hx       |    8 +++---
 hmp.c                 |   56 +++++++++++++++++++++++++++++++++++++++++
 hmp.h                 |    1 +
 monitor.c             |   67 ++++++++++++++-----------------------------------
 qapi-schema.json      |   47 ++++++++++++++++++++++++++++++++++
 qerror.c              |    4 +++
 qerror.h              |    3 ++
 qmp-commands.hx       |   27 +++++++++++++++++++
 scripts/qapi-types.py |   33 +++++++++++++++++++++---
 scripts/qapi-visit.py |   14 ++++++---
 10 files changed, 199 insertions(+), 61 deletions(-)

[Qemu-devel] [PATCH v2 1/6] qerror: add QERR_OVERFLOW

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 5092fe7..b66af09 100644
--- a/qerror.c
+++ b/qerror.c
@@ -172,6 +172,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Parameter '%(name)' expects %(expected)",
     },
     {
+        .error_fmt = QERR_OVERFLOW,
+        .desc      = "Input is overflow",
+    },
+    {
         .error_fmt = QERR_INVALID_PASSWORD,
         .desc      = "Password incorrect",
     },
diff --git a/qerror.h b/qerror.h
index 4cbba48..dfe9c89 100644
--- a/qerror.h
+++ b/qerror.h
@@ -151,6 +151,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_INVALID_PARAMETER_VALUE \
     "{ 'class': 'InvalidParameterValue', 'data': { 'name': %s, 'expected': %s } }"
 
+#define QERR_OVERFLOW \
+    "{ 'class': 'Overflow', 'data': {} }"
+
 #define QERR_INVALID_PASSWORD \
     "{ 'class': 'InvalidPassword', 'data': {} }"
 
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/6] fix doc of using raw values with sendkey

[Amos Kong]

(qemu) sendkey a
(qemu) sendkey 0x1e
(qemu) sendkey #0x1e
 unknown key: '#0x1e'

The last command doesn't work, '#' is not request before raw values.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hmp-commands.hx |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 18cb415..af18156 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -513,8 +513,8 @@ STEXI
 @findex sendkey
 
 Send @var{keys} to the emulator. @var{keys} could be the name of the
-key or @code{#} followed by the raw value in either decimal or hexadecimal
-format. Use @code{-} to press several keys simultaneously. Example:
+key or the raw value in either decimal or hexadecimal format. Use
+@code{-} to press several keys simultaneously. Example:
 @example
 sendkey ctrl-alt-f1
 @end example
-- 
1.7.1

[Qemu-devel] [PATCH v2 3/6] rename keyname '<' to 'less'

[Amos Kong]

There are many maps of keycode 0x56 in pc-bios/keymaps/*
  pc-bios/keymaps/common:less 0x56
  pc-bios/keymaps/common:greater 0x56 shift
  pc-bios/keymaps/common:bar 0x56 altgr
  pc-bios/keymaps/common:brokenbar 0x56 shift altgr

This patch just rename '<' to 'less', QAPI might add new
variable by adding a prefix to keyname, '$PREFIX_<' is not
available, '$PREFIX_less' is ok.

For compatibility, convert user inputted '<' to 'less'.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 monitor.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 12a6fe2..ecfcaa4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1302,7 +1302,7 @@ static const KeyDef key_defs[] = {
     { 0x48, "kp_8" },
     { 0x49, "kp_9" },
 
-    { 0x56, "<" },
+    { 0x56, "less" },
 
     { 0x57, "f11" },
     { 0x58, "f12" },
@@ -1406,6 +1406,13 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
                 monitor_printf(mon, "too many keys\n");
                 return;
             }
+
+            /* Be compatible with old interface, convert user inputted "<" */
+            if (!strcmp(keyname_buf, "<")) {
+                pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
+                keyname_len = 4;
+            }
+
             keyname_buf[keyname_len] = 0;
             keycode = get_keycode(keyname_buf);
             if (keycode < 0) {
-- 
1.7.1

[Qemu-devel] [PATCH v2 4/6] hmp: rename arguments

[Amos Kong]

Rename 'string' to 'keys', rename 'hold_time' to 'hold-time'

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hmp-commands.hx |    2 +-
 monitor.c       |   14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index af18156..18b8e31 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -502,7 +502,7 @@ ETEXI
 
     {
         .name       = "sendkey",
-        .args_type  = "string:s,hold_time:i?",
+        .args_type  = "keys: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,
diff --git a/monitor.c b/monitor.c
index ecfcaa4..536d9dd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1382,9 +1382,9 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
     char keyname_buf[16];
     char *separator;
     int keyname_len, keycode, i;
-    const char *string = qdict_get_str(qdict, "string");
-    int has_hold_time = qdict_haskey(qdict, "hold_time");
-    int hold_time = qdict_get_try_int(qdict, "hold_time", -1);
+    const char *keys = qdict_get_str(qdict, "keys");
+    int has_hold_time = qdict_haskey(qdict, "hold-time");
+    int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
 
     if (nb_pending_keycodes > 0) {
         qemu_del_timer(key_timer);
@@ -1394,10 +1394,10 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
         hold_time = 100;
     i = 0;
     while (1) {
-        separator = strchr(string, '-');
-        keyname_len = separator ? separator - string : strlen(string);
+        separator = strchr(keys, '-');
+        keyname_len = separator ? separator - keys : strlen(keys);
         if (keyname_len > 0) {
-            pstrcpy(keyname_buf, sizeof(keyname_buf), string);
+            pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
             if (keyname_len > sizeof(keyname_buf) - 1) {
                 monitor_printf(mon, "invalid key: '%s...'\n", keyname_buf);
                 return;
@@ -1423,7 +1423,7 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
         }
         if (!separator)
             break;
-        string = separator + 1;
+        keys = separator + 1;
     }
     nb_pending_keycodes = i;
     /* key down events */
-- 
1.7.1

[Qemu-devel] [PATCH v2 5/6] qapi: generate list struct and visit_list for enum

[Amos Kong]

Currently, if define an 'enum' and use it in one command's data,
List struct for enum could not be generated, but it's used in
qmp function.

For example: KeyCodesList could not be generated.
>>> qapi-schema.json:
{ 'enum': 'KeyCodes',
  'data': [ 'shift', 'alt' ... ] }
{ 'command': 'sendkey',
  'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }

>>> qmp-command.h:
void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
hold_time, Error **errp);

This patch makes qapi can generate List struct for enum.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
 scripts/qapi-visit.py |   14 +++++++++-----
 2 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4a734f5..c9641fb 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -16,17 +16,36 @@ import os
 import getopt
 import errno
 
-def generate_fwd_struct(name, members):
-    return mcgen('''
+def generate_fwd_struct(name, members, enum=False):
+    ret = ""
+    if not enum:
+        ret += mcgen('''
 typedef struct %(name)s %(name)s;
 
+''',
+                     name=name)
+    ret += mcgen('''
 typedef struct %(name)sList
 {
-    %(name)s *value;
+''',
+                     name=name)
+    if enum:
+        ret += mcgen('''
+         %(name)s value;
+''',
+                     name=name)
+    else:
+        ret += mcgen('''
+         %(name)s * value;
+''',
+                     name=name)
+
+    ret += mcgen('''
     struct %(name)sList *next;
 } %(name)sList;
 ''',
                  name=name)
+    return ret
 
 def generate_struct(structname, fieldname, members):
     ret = mcgen('''
@@ -265,7 +284,8 @@ for expr in exprs:
     if expr.has_key('type'):
         ret += generate_fwd_struct(expr['type'], expr['data'])
     elif expr.has_key('enum'):
-        ret += generate_enum(expr['enum'], expr['data'])
+        ret += generate_enum(expr['enum'], expr['data']) + "\n"
+        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
         fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
     elif expr.has_key('union'):
         ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
@@ -289,6 +309,11 @@ for expr in exprs:
         fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
         ret += generate_type_cleanup_decl(expr['union'])
         fdef.write(generate_type_cleanup(expr['union']) + "\n")
+    elif expr.has_key('enum'):
+        ret += generate_type_cleanup_decl(expr['enum'] + "List")
+        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
+        ret += generate_type_cleanup_decl(expr['enum'])
+        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
     else:
         continue
     fdecl.write(ret)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 8d4e94a..e44edfa 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -81,7 +81,7 @@ end:
 ''')
     return ret
 
-def generate_visit_list(name, members):
+def generate_visit_list(name, members, enum=False):
     return mcgen('''
 
 void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
@@ -160,12 +160,14 @@ end:
 
     return ret
 
-def generate_declaration(name, members, genlist=True):
-    ret = mcgen('''
+def generate_declaration(name, members, genlist=True, enum=False):
+    ret = ""
+    if not enum:
+        ret = mcgen('''
 
 void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
 ''',
-                name=name)
+                    name=name)
 
     if genlist:
         ret += mcgen('''
@@ -293,10 +295,12 @@ for expr in exprs:
         ret += generate_declaration(expr['union'], expr['data'])
         fdecl.write(ret)
     elif expr.has_key('enum'):
-        ret = generate_visit_enum(expr['enum'], expr['data'])
+        ret = generate_visit_list(expr['enum'], expr['data'], True)
+        ret += generate_visit_enum(expr['enum'], expr['data'])
         fdef.write(ret)
 
         ret = generate_decl_enum(expr['enum'], expr['data'])
+        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
         fdecl.write(ret)
 
 fdecl.write('''
-- 
1.7.1

[Qemu-devel] [PATCH v2 6/6] qapi: convert sendkey

[Amos Kong]

Convert 'sendkey' to use QAPI. do_sendkey() depends on some
variables/functions in monitor.c, so reserve qmp_sendkey()
to monitor.c

key_defs[] in monitor.c is the mapping of key name to keycode,
Keys' order in the enmu and key_defs[] is same.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hmp-commands.hx  |    2 +-
 hmp.c            |   56 +++++++++++++++++++++++++++++++++++++++++++
 hmp.h            |    1 +
 monitor.c        |   70 +++++++++++++----------------------------------------
 qapi-schema.json |   47 ++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   27 ++++++++++++++++++++
 6 files changed, 149 insertions(+), 54 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 18b8e31..afbfa61 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -505,7 +505,7 @@ ETEXI
         .args_type  = "keys: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,
+        .mhandler.cmd = hmp_sendkey,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index bb0952e..06a6686 100644
--- a/hmp.c
+++ b/hmp.c
@@ -16,6 +16,7 @@
 #include "hmp.h"
 #include "qemu-timer.h"
 #include "qmp-commands.h"
+#include "qapi-types.h"
 
 static void hmp_handle_error(Monitor *mon, Error **errp)
 {
@@ -947,3 +948,58 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
     qmp_device_del(id, &err);
     hmp_handle_error(mon, &err);
 }
+
+static int get_index(const char *key)
+{
+    int i;
+
+    for (i = 0; KeyCodes_lookup[i] != NULL; i++) {
+        if (!strcmp(key, KeyCodes_lookup[i]))
+            return i;
+    }
+    return -1;
+}
+
+void hmp_sendkey(Monitor *mon, const QDict *qdict)
+{
+    const char *keys = qdict_get_str(qdict, "keys");
+    KeyCodesList *keylist, *head = NULL, *tmp = NULL;
+    int has_hold_time = qdict_haskey(qdict, "hold-time");
+    int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
+    Error *err = NULL;
+    char keyname_buf[16];
+
+    char *separator;
+    int keyname_len;
+
+    while (1) {
+        separator = strchr(keys, '-');
+        keyname_len = separator ? separator - keys : strlen(keys);
+        pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
+
+        /* Be compatible with old interface, convert user inputted "<" */
+        if (!strcmp(keyname_buf, "<")) {
+            pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
+            keyname_len = 4;
+        }
+        keyname_buf[keyname_len] = 0;
+
+        keylist = g_malloc0(sizeof(*keylist));
+        keylist->value = get_index(keyname_buf);
+        keylist->next = NULL;
+
+        if (tmp)
+            tmp->next = keylist;
+        tmp = keylist;
+        if (!head)
+            head = keylist;
+
+        if (!separator)
+            break;
+        keys = separator + 1;
+    }
+
+    qmp_sendkey(head, has_hold_time, hold_time, &err);
+    qapi_free_KeyCodesList(keylist);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 443b812..40de72c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -61,5 +61,6 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
 void hmp_device_del(Monitor *mon, const QDict *qdict);
+void hmp_sendkey(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index 536d9dd..2858ac0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1341,24 +1341,6 @@ static const KeyDef key_defs[] = {
     { 0, NULL },
 };
 
-static int get_keycode(const char *key)
-{
-    const KeyDef *p;
-    char *endp;
-    int ret;
-
-    for(p = key_defs; p->name != NULL; p++) {
-        if (!strcmp(key, p->name))
-            return p->keycode;
-    }
-    if (strstart(key, "0x", NULL)) {
-        ret = strtoul(key, &endp, 0);
-        if (*endp == '\0' && ret >= 0x01 && ret <= 0xff)
-            return ret;
-    }
-    return -1;
-}
-
 #define MAX_KEYCODES 16
 static uint8_t keycodes[MAX_KEYCODES];
 static int nb_pending_keycodes;
@@ -1377,14 +1359,12 @@ static void release_keys(void *opaque)
     }
 }
 
-static void do_sendkey(Monitor *mon, const QDict *qdict)
+void qmp_sendkey(KeyCodesList *keys, bool has_hold_time, int64_t hold_time,
+                 Error **errp)
 {
     char keyname_buf[16];
-    char *separator;
-    int keyname_len, keycode, i;
-    const char *keys = qdict_get_str(qdict, "keys");
-    int has_hold_time = qdict_haskey(qdict, "hold-time");
-    int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
+    int keycode, i;
+    KeyCodesList *entry = keys;
 
     if (nb_pending_keycodes > 0) {
         qemu_del_timer(key_timer);
@@ -1392,39 +1372,23 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
     }
     if (!has_hold_time)
         hold_time = 100;
-    i = 0;
-    while (1) {
-        separator = strchr(keys, '-');
-        keyname_len = separator ? separator - keys : strlen(keys);
-        if (keyname_len > 0) {
-            pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
-            if (keyname_len > sizeof(keyname_buf) - 1) {
-                monitor_printf(mon, "invalid key: '%s...'\n", keyname_buf);
-                return;
-            }
-            if (i == MAX_KEYCODES) {
-                monitor_printf(mon, "too many keys\n");
-                return;
-            }
 
-            /* Be compatible with old interface, convert user inputted "<" */
-            if (!strcmp(keyname_buf, "<")) {
-                pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
-                keyname_len = 4;
-            }
+    i = 0;
+    while (entry != NULL) {
+        if (entry->value > sizeof(key_defs) / sizeof(*(key_defs))) {
+            error_set(errp, QERR_INVALID_PARAMETER, keyname_buf);
+            return;
+        }
 
-            keyname_buf[keyname_len] = 0;
-            keycode = get_keycode(keyname_buf);
-            if (keycode < 0) {
-                monitor_printf(mon, "unknown key: '%s'\n", keyname_buf);
-                return;
-            }
-            keycodes[i++] = keycode;
+        if (i == MAX_KEYCODES) {
+            error_set(errp, QERR_OVERFLOW);
+            return;
         }
-        if (!separator)
-            break;
-        keys = separator + 1;
+
+        keycodes[i++] = key_defs[entry->value].keycode;
+        entry = entry->next;
     }
+
     nb_pending_keycodes = i;
     /* key down events */
     for (i = 0; i < nb_pending_keycodes; i++) {
diff --git a/qapi-schema.json b/qapi-schema.json
index 2ca7195..8fa0cb7 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1755,3 +1755,50 @@
 # Since: 0.14.0
 ##
 { 'command': 'device_del', 'data': {'id': 'str'} }
+
+##
+# @KeyCodes:
+#
+# An enumeration of key name.
+#
+# This is used by the sendkey command.
+#
+# Since: 0.14.0
+##
+{ 'enum': 'KeyCodes',
+  'data': [ 'shift', 'shift_r', 'alt', 'alt_r', 'altgr', 'altgr_r', 'ctrl',
+            'ctrl_r', 'menu', 'esc', '1', '2', '3', '4', '5', '6', '7', '8',
+            '9', '0', 'minus', 'equal', 'backspace', 'tab', 'q', 'w', 'e',
+            'r', 't', 'y', 'u', 'i', 'o', 'p', 'bracket_left', 'bracket_right',
+            'ret', 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'semicolon',
+            'apostrophe', 'grave_accent', 'backslash', 'z', 'x', 'c', 'v', 'b',
+            'n', 'm', 'comma', 'dot', 'slash', 'asterisk', 'spc', 'caps_lock',
+            'f1', 'f2', 'f3', 'f4', 'f5', 'f6', 'f7', 'f8', 'f9', 'f10',
+            'num_lock', 'scroll_lock', 'kp_divide', 'kp_multiply',
+            'kp_subtract', 'kp_add', 'kp_enter', 'kp_decimal', 'sysrq', 'kp_0',
+            'kp_1', 'kp_2', 'kp_3', 'kp_4', 'kp_5', 'kp_6', 'kp_7', 'kp_8',
+            'kp_9', 'less', 'f11', 'f12', 'print', 'home', 'pgup', 'pgdn', 'end',
+            'left', 'up', 'down', 'right', 'insert', 'delete', 'stop', 'again',
+            'props', 'undo', 'front', 'copy', 'open', 'paste', 'find', 'cut',
+             'lf', 'help', 'meta_l', 'meta_r', 'compose' ] }
+
+##
+# @sendkey:
+#
+# Send keys to VM.
+#
+# @keys: key sequence
+# @hold-time: time to delay key up events, milliseconds
+#
+# Returns: Nothing on success
+#          If key is unknown or redundant, QERR_INVALID_PARAMETER
+#          If keys number is over the limitation, QERR_OVERFLOW
+#
+# Notes: Send keys to the emulator. Keys could be the name of the
+#        key or the raw value in either decimal or hexadecimal  format. Use
+#        "-" to press several keys simultaneously.
+#
+# Since: 0.14.0
+##
+{ 'command': 'sendkey',
+  'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db980fa..3692805 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -335,6 +335,33 @@ Example:
 EQMP
 
     {
+        .name       = "sendkey",
+        .args_type  = "keys:O,hold-time:i?",
+        .mhandler.cmd_new = qmp_marshal_input_sendkey,
+    },
+
+SQMP
+sendkey
+----------
+
+Send keys to VM.
+
+Arguments:
+
+keys array:
+    - "key": key sequence (json-string)
+
+- hold-time: time to delay key up events, miliseconds (josn-int, optional)
+
+Example:
+
+-> { "execute": "sendkey",
+     "arguments": { 'keys': [ 'ctrl', 'alt', 'delete' ] } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "cpu",
         .args_type  = "index:i",
         .mhandler.cmd_new = qmp_marshal_input_cpu,
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 0/6] convert sendkey to qapi

[Amos Kong]

----- Original Message -----
> This series converted 'sendkey' command to qapi.
> 
> Amos Kong (6):
>   qerror: add QERR_OVERFLOW
>   fix doc of using raw values with sendkey
>   rename keyname '<' to 'less'
>   hmp: rename arguments
>   qapi: generate list struct and visit_list for enum
>   qapi: convert sendkey


Changes from v1:
- using a JSON array for the key names
- rename new error to 'QERR_OVERFLOW'
- fix command descriptions 
- qapi: generate list struct for enum
- add '<' fixing

 
>  hmp-commands.hx       |    8 +++---
>  hmp.c                 |   56
>  +++++++++++++++++++++++++++++++++++++++++
>  hmp.h                 |    1 +
>  monitor.c             |   67
>  ++++++++++++++-----------------------------------
>  qapi-schema.json      |   47 ++++++++++++++++++++++++++++++++++
>  qerror.c              |    4 +++
>  qerror.h              |    3 ++
>  qmp-commands.hx       |   27 +++++++++++++++++++
>  scripts/qapi-types.py |   33 +++++++++++++++++++++---
>  scripts/qapi-visit.py |   14 ++++++---
>  10 files changed, 199 insertions(+), 61 deletions(-)
> 
> 

Re: [Qemu-devel] [PATCH v2 1/6] qerror: add QERR_OVERFLOW

[Anthony Liguori]

On 06/02/2012 06:54 AM, Amos Kong wrote:
> Signed-off-by: Amos Kong<akong@redhat.com>

I think QERR_INVALID_PARAMETER_VALUE is a more logical choice for the error 
you're generating.

Regards,

Anthony Liguori

> ---
>   qerror.c |    4 ++++
>   qerror.h |    3 +++
>   2 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/qerror.c b/qerror.c
> index 5092fe7..b66af09 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -172,6 +172,10 @@ static const QErrorStringTable qerror_table[] = {
>           .desc      = "Parameter '%(name)' expects %(expected)",
>       },
>       {
> +        .error_fmt = QERR_OVERFLOW,
> +        .desc      = "Input is overflow",
> +    },
> +    {
>           .error_fmt = QERR_INVALID_PASSWORD,
>           .desc      = "Password incorrect",
>       },
> diff --git a/qerror.h b/qerror.h
> index 4cbba48..dfe9c89 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -151,6 +151,9 @@ QError *qobject_to_qerror(const QObject *obj);
>   #define QERR_INVALID_PARAMETER_VALUE \
>       "{ 'class': 'InvalidParameterValue', 'data': { 'name': %s, 'expected': %s } }"
>
> +#define QERR_OVERFLOW \
> +    "{ 'class': 'Overflow', 'data': {} }"
> +
>   #define QERR_INVALID_PASSWORD \
>       "{ 'class': 'InvalidPassword', 'data': {} }"
>

Re: [Qemu-devel] [PATCH v2 6/6] qapi: convert sendkey

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]

On 06/01/2012 04:54 PM, Amos Kong wrote:
> Convert 'sendkey' to use QAPI. do_sendkey() depends on some
> variables/functions in monitor.c, so reserve qmp_sendkey()
> to monitor.c
> 
> key_defs[] in monitor.c is the mapping of key name to keycode,
> Keys' order in the enmu and key_defs[] is same.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -1755,3 +1755,50 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'device_del', 'data': {'id': 'str'} }
> +
> +##
> +# @KeyCodes:
> +#
> +# An enumeration of key name.
> +#
> +# This is used by the sendkey command.
> +#
> +# Since: 0.14.0

Really?  Or is this enum since 1.2?

> +
> +##
> +# @sendkey:
> +#
> +# Send keys to VM.
> +#
> +# @keys: key sequence
> +# @hold-time: time to delay key up events, milliseconds
> +#
> +# Returns: Nothing on success
> +#          If key is unknown or redundant, QERR_INVALID_PARAMETER
> +#          If keys number is over the limitation, QERR_OVERFLOW
> +#
> +# Notes: Send keys to the emulator. Keys could be the name of the
> +#        key or the raw value in either decimal or hexadecimal  format. Use
> +#        "-" to press several keys simultaneously.

These notes don't really correspond to the QMP interface of passing in a
JSON array of simultaneous keys to press.

> +#
> +# Since: 0.14.0

Again, shouldn't this be 1.2?


> +SQMP
> +sendkey
> +----------
> +
> +Send keys to VM.
> +
> +Arguments:
> +
> +keys array:
> +    - "key": key sequence (json-string)
> +
> +- hold-time: time to delay key up events, miliseconds (josn-int, optional)

s/miliseconds/milliseconds/

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v2 1/6] qerror: add MAX_KEYCODES 16

[Amos Kong]

----- Original Message -----
> On 06/02/2012 06:54 AM, Amos Kong wrote:
> > Signed-off-by: Amos Kong<akong@redhat.com>
> 
> I think QERR_INVALID_PARAMETER_VALUE is a more logical choice for the
> error
> you're generating.

I used this new error in [PATCH 6/6], if user inputs more than
16 keys once, this error will occur, even if all the keys are
valid.
If user sees the new error, they will identify that keys number
should be reduced. But 'QERR_INVALID_PARAMETER_VALUE' could not
pass this information to user.


Amos.

> Regards,
> 
> Anthony Liguori
> 
> > ---
> >   qerror.c |    4 ++++
> >   qerror.h |    3 +++
> >   2 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/qerror.c b/qerror.c
> > index 5092fe7..b66af09 100644
> > --- a/qerror.c
> > +++ b/qerror.c
> > @@ -172,6 +172,10 @@ static const QErrorStringTable qerror_table[]
> > = {
> >           .desc      = "Parameter '%(name)' expects %(expected)",
> >       },
> >       {
> > +        .error_fmt = QERR_OVERFLOW,
> > +        .desc      = "Input is overflow",
> > +    },
> > +    {
> >           .error_fmt = QERR_INVALID_PASSWORD,
> >           .desc      = "Password incorrect",
> >       },
> > diff --git a/qerror.h b/qerror.h
> > index 4cbba48..dfe9c89 100644
> > --- a/qerror.h
> > +++ b/qerror.h
> > @@ -151,6 +151,9 @@ QError *qobject_to_qerror(const QObject *obj);
> >   #define QERR_INVALID_PARAMETER_VALUE \
> >       "{ 'class': 'InvalidParameterValue', 'data': { 'name': %s,
> >       'expected': %s } }"
> >
> > +#define QERR_OVERFLOW \
> > +    "{ 'class': 'Overflow', 'data': {} }"
> > +
> >   #define QERR_INVALID_PASSWORD \
> >       "{ 'class': 'InvalidPassword', 'data': {} }"
> >
> 
> 

Re: [Qemu-devel] [PATCH v2 6/6] qapi: convert sendkey

[Amos Kong]

Hello Eric,

Thanks for your comments.

----- Original Message -----
> On 06/01/2012 04:54 PM, Amos Kong wrote:
> > Convert 'sendkey' to use QAPI. do_sendkey() depends on some
> > variables/functions in monitor.c, so reserve qmp_sendkey()
> > to monitor.c
> > 
> > key_defs[] in monitor.c is the mapping of key name to keycode,
> > Keys' order in the enmu and key_defs[] is same.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -1755,3 +1755,50 @@
> >  # Since: 0.14.0
> >  ##
> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> > +
> > +##
> > +# @KeyCodes:
> > +#
> > +# An enumeration of key name.
> > +#
> > +# This is used by the sendkey command.
> > +#
> > +# Since: 0.14.0
> 
> Really?  Or is this enum since 1.2?

Yeah, it should be 1.2

> 
> > +
> > +##
> > +# @sendkey:
> > +#
> > +# Send keys to VM.
> > +#
> > +# @keys: key sequence
> > +# @hold-time: time to delay key up events, milliseconds
> > +#
> > +# Returns: Nothing on success
> > +#          If key is unknown or redundant, QERR_INVALID_PARAMETER
> > +#          If keys number is over the limitation, QERR_OVERFLOW
> > +#
> > +# Notes: Send keys to the emulator. Keys could be the name of the
> > +#        key or the raw value in either decimal or hexadecimal format. Use
> > +#        "-" to press several keys simultaneously.
> 
> These notes don't really correspond to the QMP interface of passing
> in a JSON array of simultaneous keys to press.


# Notes: Send keys to the emulator. Keys could be the name of the
#        key or the raw value in either decimal or hexadecimal format. Use
#        a JSON array to press several keys simultaneously.



Ho, I found another bug in my code, key in decimal or hexadecimal
format is not supported. I will fix it.

 
> > +#
> > +# Since: 0.14.0
> 
> Again, shouldn't this be 1.2?

yeah, 1.2
 
> 
> > +SQMP
> > +sendkey
> > +----------
> > +
> > +Send keys to VM.
> > +
> > +Arguments:
> > +
> > +keys array:
> > +    - "key": key sequence (json-string)
> > +
> > +- hold-time: time to delay key up events, miliseconds (josn-int,
> > optional)
> 
> s/miliseconds/milliseconds/
> 
> --
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 

Re: [Qemu-devel] [PATCH v2 5/6] qapi: generate list struct and visit_list for enum

[Amos Kong]

----- Original Message -----
> Currently, if define an 'enum' and use it in one command's data,
> List struct for enum could not be generated, but it's used in
> qmp function.
> 
> For example: KeyCodesList could not be generated.
> >>> qapi-schema.json:
> { 'enum': 'KeyCodes',
>   'data': [ 'shift', 'alt' ... ] }
> { 'command': 'sendkey',
>   'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> 
> >>> qmp-command.h:
> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> hold_time, Error **errp);
> 
> This patch makes qapi can generate List struct for enum.


Anthony, Mike, any comments on this patch?


> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>  scripts/qapi-visit.py |   14 +++++++++-----
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4a734f5..c9641fb 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,17 +16,36 @@ import os
>  import getopt
>  import errno
>  
> -def generate_fwd_struct(name, members):
> -    return mcgen('''
> +def generate_fwd_struct(name, members, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret += mcgen('''
>  typedef struct %(name)s %(name)s;
>  
> +''',
> +                     name=name)
> +    ret += mcgen('''
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +''',
> +                     name=name)
> +    if enum:
> +        ret += mcgen('''
> +         %(name)s value;
> +''',
> +                     name=name)
> +    else:
> +        ret += mcgen('''
> +         %(name)s * value;
> +''',
> +                     name=name)
> +
> +    ret += mcgen('''
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
>                   name=name)
> +    return ret
>  
>  def generate_struct(structname, fieldname, members):
>      ret = mcgen('''
> @@ -265,7 +284,8 @@ for expr in exprs:
>      if expr.has_key('type'):
>          ret += generate_fwd_struct(expr['type'], expr['data'])
>      elif expr.has_key('enum'):
> -        ret += generate_enum(expr['enum'], expr['data'])
> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>          fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>      elif expr.has_key('union'):
>          ret += generate_fwd_struct(expr['union'], expr['data']) +
>          "\n"
> @@ -289,6 +309,11 @@ for expr in exprs:
>          fdef.write(generate_type_cleanup(expr['union'] + "List") +
>          "\n")
>          ret += generate_type_cleanup_decl(expr['union'])
>          fdef.write(generate_type_cleanup(expr['union']) + "\n")
> +    elif expr.has_key('enum'):
> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") +
> "\n")
> +        ret += generate_type_cleanup_decl(expr['enum'])
> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
>      else:
>          continue
>      fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8d4e94a..e44edfa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,7 +81,7 @@ end:
>  ''')
>      return ret
>  
> -def generate_visit_list(name, members):
> +def generate_visit_list(name, members, enum=False):
>      return mcgen('''
>  
>  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const
>  char *name, Error **errp)
> @@ -160,12 +160,14 @@ end:
>  
>      return ret
>  
> -def generate_declaration(name, members, genlist=True):
> -    ret = mcgen('''
> +def generate_declaration(name, members, genlist=True, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret = mcgen('''
>  
>  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char
>  *name, Error **errp);
>  ''',
> -                name=name)
> +                    name=name)
>  
>      if genlist:
>          ret += mcgen('''
> @@ -293,10 +295,12 @@ for expr in exprs:
>          ret += generate_declaration(expr['union'], expr['data'])
>          fdecl.write(ret)
>      elif expr.has_key('enum'):
> -        ret = generate_visit_enum(expr['enum'], expr['data'])
> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>          fdef.write(ret)
>  
>          ret = generate_decl_enum(expr['enum'], expr['data'])
> +        ret += generate_declaration(expr['enum'], expr['data'],
> enum=True)
>          fdecl.write(ret)
>  
>  fdecl.write('''
> --
> 1.7.1
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 6/6] qapi: convert sendkey

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

On 06/05/2012 08:55 AM, Amos Kong wrote:

>>> +# @sendkey:
>>> +#
>>> +# Send keys to VM.
>>> +#
>>> +# @keys: key sequence
>>> +# @hold-time: time to delay key up events, milliseconds
>>> +#
>>> +# Returns: Nothing on success
>>> +#          If key is unknown or redundant, QERR_INVALID_PARAMETER
>>> +#          If keys number is over the limitation, QERR_OVERFLOW
>>> +#
>>> +# Notes: Send keys to the emulator. Keys could be the name of the
>>> +#        key or the raw value in either decimal or hexadecimal format. Use
>>> +#        "-" to press several keys simultaneously.
>>
>> These notes don't really correspond to the QMP interface of passing
>> in a JSON array of simultaneous keys to press.
> 
> 
> # Notes: Send keys to the emulator. Keys could be the name of the
> #        key or the raw value in either decimal or hexadecimal format. Use
> #        a JSON array to press several keys simultaneously.
> 
> 
> 
> Ho, I found another bug in my code, key in decimal or hexadecimal
> format is not supported. I will fix it.

How do you plan to support decimal in QMP?  I don't think it's possible,
at least not without still keeping a JSON array of keys to press.  On
the other hand, I'm not sure if we need to worry about that.  Let me
explain:

Let's suppose the QMP interface is symbolic only.

In the HMP interface, _you_ can use the lookup list to convert decimal
to key name, so that the HMP interface is a wrapper around the QMP
interface, but everything is symbolic by the time we get that far.

In the libvirt case, where libvirt talks directly to QMP, libvirt also
has the same table for looking up keysyms vs. values (in fact, libvirt
already uses that table to convert between different keysets, so for an
example, the libvirt user can specify a command using xt_kbd or usb or
... mappings, which libvirt then converts into the mapping desired by
qemu).  Since libvirt already consults a table, libvirt can ensure that
the table has the proper QMP symbolic names in that table.

Finally, I know there was a patch proposed by Dan Berrange a while back
to let both libvirt and qemu share a common database of keyset names and
corresponding integer mappings.  I don't remember if it was applied to
qemu; if not, it should be revived and used at this time.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v2 6/6] qapi: convert sendkey

[Amos Kong]

On 05/06/12 23:05, Eric Blake wrote:
> On 06/05/2012 08:55 AM, Amos Kong wrote:
>
>>>> +# @sendkey:
>>>> +#
>>>> +# Send keys to VM.
>>>> +#
>>>> +# @keys: key sequence
>>>> +# @hold-time: time to delay key up events, milliseconds
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#          If key is unknown or redundant, QERR_INVALID_PARAMETER
>>>> +#          If keys number is over the limitation, QERR_OVERFLOW
>>>> +#
>>>> +# Notes: Send keys to the emulator. Keys could be the name of the
>>>> +#        key or the raw value in either decimal or hexadecimal format. Use
>>>> +#        "-" to press several keys simultaneously.
>>>
>>> These notes don't really correspond to the QMP interface of passing
>>> in a JSON array of simultaneous keys to press.
>>
>>
>> # Notes: Send keys to the emulator. Keys could be the name of the
>> #        key or the raw value in either decimal or hexadecimal format. Use
>> #        a JSON array to press several keys simultaneously.
>>
>>
>>
>> Ho, I found another bug in my code, key in decimal or hexadecimal
>> format is not supported. I will fix it.
>
> How do you plan to support decimal in QMP?

In old do_sendkey(), only key-name and hexadecimal were supported,
the description needs to be fixed.


>   I don't think it's possible,
> at least not without still keeping a JSON array of keys to press.  On
> the other hand, I'm not sure if we need to worry about that.  Let me
> explain:
>
> Let's suppose the QMP interface is symbolic only.
>
> In the HMP interface, _you_ can use the lookup list to convert decimal
> to key name, so that the HMP interface is a wrapper around the QMP
> interface, but everything is symbolic by the time we get that far.
>
> In the libvirt case, where libvirt talks directly to QMP,

It seems we can only support key-name for QMP, and support 
key-name/hexadecimal
for HMP. Is it acceptable?


>  libvirt also
> has the same table for looking up keysyms vs. values (in fact, libvirt
> already uses that table to convert between different keysets, so for an
> example, the libvirt user can specify a command using xt_kbd or usb or
> ... mappings, which libvirt then converts into the mapping desired by
> qemu).  Since libvirt already consults a table, libvirt can ensure that
> the table has the proper QMP symbolic names in that table.


> Finally, I know there was a patch proposed by Dan Berrange a while back
> to let both libvirt and qemu share a common database of keyset names and
> corresponding integer mappings.  I don't remember if it was applied to
> qemu; if not, it should be revived and used at this time.
>

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 6/6] qapi: convert sendkey

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On 06/06/2012 01:13 AM, Amos Kong wrote:

>>> Ho, I found another bug in my code, key in decimal or hexadecimal
>>> format is not supported. I will fix it.
>>
>> How do you plan to support decimal in QMP?
> 
> In old do_sendkey(), only key-name and hexadecimal were supported,
> the description needs to be fixed.
> 

> 
> It seems we can only support key-name for QMP, and support
> key-name/hexadecimal
> for HMP. Is it acceptable?

Yes, that was my argument for bare minimum support - anyone using QMP
(like libvirt) will have to be smart enough to use key-name only, while
anyone using HMP will be able to use hex because HMP will convert hex to
key-name at the appropriate time before calling into QMP.

Of course, being able to support hex from QMP would be a nice feature,
but I'm not sure how to add it in.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v2 2/6] fix doc of using raw values with sendkey

[Luiz Capitulino]

On Sat,  2 Jun 2012 06:54:24 +0800
Amos Kong <akong@redhat.com> wrote:

> (qemu) sendkey a
> (qemu) sendkey 0x1e
> (qemu) sendkey #0x1e
>  unknown key: '#0x1e'
> 
> The last command doesn't work, '#' is not request before raw values.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hmp-commands.hx |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 18cb415..af18156 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -513,8 +513,8 @@ STEXI
>  @findex sendkey
>  
>  Send @var{keys} to the emulator. @var{keys} could be the name of the

I think we should do:

 s/emulator/guest

Also applies to the schema doc.

> -key or @code{#} followed by the raw value in either decimal or hexadecimal
> -format. Use @code{-} to press several keys simultaneously. Example:
> +key or the raw value in either decimal or hexadecimal format. Use
> +@code{-} to press several keys simultaneously. Example:
>  @example
>  sendkey ctrl-alt-f1
>  @end example

Re: [Qemu-devel] [PATCH v2 3/6] rename keyname '<' to 'less'

[Luiz Capitulino]

On Sat,  2 Jun 2012 06:54:25 +0800
Amos Kong <akong@redhat.com> wrote:

> There are many maps of keycode 0x56 in pc-bios/keymaps/*
>   pc-bios/keymaps/common:less 0x56
>   pc-bios/keymaps/common:greater 0x56 shift
>   pc-bios/keymaps/common:bar 0x56 altgr
>   pc-bios/keymaps/common:brokenbar 0x56 shift altgr
> 
> This patch just rename '<' to 'less', QAPI might add new
> variable by adding a prefix to keyname, '$PREFIX_<' is not
> available, '$PREFIX_less' is ok.
> 
> For compatibility, convert user inputted '<' to 'less'.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  monitor.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 12a6fe2..ecfcaa4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1302,7 +1302,7 @@ static const KeyDef key_defs[] = {
>      { 0x48, "kp_8" },
>      { 0x49, "kp_9" },
>  
> -    { 0x56, "<" },
> +    { 0x56, "less" },
>  
>      { 0x57, "f11" },
>      { 0x58, "f12" },
> @@ -1406,6 +1406,13 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
>                  monitor_printf(mon, "too many keys\n");
>                  return;
>              }
> +
> +            /* Be compatible with old interface, convert user inputted "<" */
> +            if (!strcmp(keyname_buf, "<")) {
> +                pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
> +                keyname_len = 4;
> +            }

I'm not against this change, but it breaks 'sendkey <-a'

> +
>              keyname_buf[keyname_len] = 0;
>              keycode = get_keycode(keyname_buf);
>              if (keycode < 0) {

Re: [Qemu-devel] [PATCH v2 5/6] qapi: generate list struct and visit_list for enum

[Luiz Capitulino]

On Sat,  2 Jun 2012 06:54:27 +0800
Amos Kong <akong@redhat.com> wrote:

> Currently, if define an 'enum' and use it in one command's data,
> List struct for enum could not be generated, but it's used in
> qmp function.
> 
> For example: KeyCodesList could not be generated.
> >>> qapi-schema.json:
> { 'enum': 'KeyCodes',
>   'data': [ 'shift', 'alt' ... ] }
> { 'command': 'sendkey',
>   'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> 
> >>> qmp-command.h:
> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> hold_time, Error **errp);
> 
> This patch makes qapi can generate List struct for enum.

This patch does it the simple way, just like any type. It generates a enum list
type and the functions qapi_free_yourenum() and qapi_free_yourenumlist().

The qapi_free_yourenum() list ends up doing nothing, so it could be a good
idea to generate an empty body (also note that we're copying the argument's
value, this could bite us in the future).

Another point I was wondering is that, all enums will end up having the
exact same code. So maybe we could generate a default int list and use it
for all enums. Not sure it's worth it though.

Michael, Paolo, ideas?

More review comments below.


> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>  scripts/qapi-visit.py |   14 +++++++++-----
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4a734f5..c9641fb 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,17 +16,36 @@ import os
>  import getopt
>  import errno
>  
> -def generate_fwd_struct(name, members):
> -    return mcgen('''
> +def generate_fwd_struct(name, members, enum=False):

I think it's better to have generate_fwd_enum_struct().

> +    ret = ""
> +    if not enum:
> +        ret += mcgen('''
>  typedef struct %(name)s %(name)s;
>  
> +''',
> +                     name=name)
> +    ret += mcgen('''
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +''',
> +                     name=name)
> +    if enum:
> +        ret += mcgen('''
> +         %(name)s value;
> +''',
> +                     name=name)
> +    else:
> +        ret += mcgen('''
> +         %(name)s * value;
> +''',
> +                     name=name)
> +
> +    ret += mcgen('''
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
>                   name=name)
> +    return ret
>  
>  def generate_struct(structname, fieldname, members):
>      ret = mcgen('''
> @@ -265,7 +284,8 @@ for expr in exprs:
>      if expr.has_key('type'):
>          ret += generate_fwd_struct(expr['type'], expr['data'])
>      elif expr.has_key('enum'):
> -        ret += generate_enum(expr['enum'], expr['data'])
> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"

The new-line should be returned by generate_enum(). Same applies for the
occurrences below.

> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>          fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>      elif expr.has_key('union'):
>          ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> @@ -289,6 +309,11 @@ for expr in exprs:
>          fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
>          ret += generate_type_cleanup_decl(expr['union'])
>          fdef.write(generate_type_cleanup(expr['union']) + "\n")
> +    elif expr.has_key('enum'):
> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
> +        ret += generate_type_cleanup_decl(expr['enum'])
> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
>      else:
>          continue
>      fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8d4e94a..e44edfa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,7 +81,7 @@ end:
>  ''')
>      return ret
>  
> -def generate_visit_list(name, members):
> +def generate_visit_list(name, members, enum=False):
>      return mcgen('''
>  
>  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> @@ -160,12 +160,14 @@ end:
>  
>      return ret
>  
> -def generate_declaration(name, members, genlist=True):
> -    ret = mcgen('''
> +def generate_declaration(name, members, genlist=True, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret = mcgen('''
>  
>  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
>  ''',
> -                name=name)
> +                    name=name)

Why this change?

>  
>      if genlist:
>          ret += mcgen('''
> @@ -293,10 +295,12 @@ for expr in exprs:
>          ret += generate_declaration(expr['union'], expr['data'])
>          fdecl.write(ret)
>      elif expr.has_key('enum'):
> -        ret = generate_visit_enum(expr['enum'], expr['data'])
> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>          fdef.write(ret)
>  
>          ret = generate_decl_enum(expr['enum'], expr['data'])
> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
>          fdecl.write(ret)
>  
>  fdecl.write('''

Re: [Qemu-devel] [PATCH v2 3/6] rename keyname '<' to 'less'

[Amos Kong]

----- Original Message -----
> On Sat,  2 Jun 2012 06:54:25 +0800
> Amos Kong <akong@redhat.com> wrote:
> 
> > There are many maps of keycode 0x56 in pc-bios/keymaps/*
> >   pc-bios/keymaps/common:less 0x56
> >   pc-bios/keymaps/common:greater 0x56 shift
> >   pc-bios/keymaps/common:bar 0x56 altgr
> >   pc-bios/keymaps/common:brokenbar 0x56 shift altgr
> > 
> > This patch just rename '<' to 'less', QAPI might add new
> > variable by adding a prefix to keyname, '$PREFIX_<' is not
> > available, '$PREFIX_less' is ok.
> > 
> > For compatibility, convert user inputted '<' to 'less'.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  monitor.c |    9 ++++++++-
> >  1 files changed, 8 insertions(+), 1 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 12a6fe2..ecfcaa4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1302,7 +1302,7 @@ static const KeyDef key_defs[] = {
> >      { 0x48, "kp_8" },
> >      { 0x49, "kp_9" },
> >  
> > -    { 0x56, "<" },
> > +    { 0x56, "less" },
> >  
> >      { 0x57, "f11" },
> >      { 0x58, "f12" },
> > @@ -1406,6 +1406,13 @@ static void do_sendkey(Monitor *mon, const
> > QDict *qdict)
> >                  monitor_printf(mon, "too many keys\n");
> >                  return;
> >              }
> > +
> > +            /* Be compatible with old interface, convert user
> > inputted "<" */
> > +            if (!strcmp(keyname_buf, "<")) {
> > +                pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
> > +                keyname_len = 4;
> > +            }
> 
> I'm not against this change, but it breaks 'sendkey <-a'

Thanks for catching this, already fixed:

@@ -978,7 +1141,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
         pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
 
         /* Be compatible with old interface, convert user inputted "<" */
-        if (!strcmp(keyname_buf, "<")) {
+        if (!strncmp(keyname_buf, "<", 1) && keyname_len == 1) {
             pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
             keyname_len = 4;
         }


> 
> > +
> >              keyname_buf[keyname_len] = 0;
> >              keycode = get_keycode(keyname_buf);
> >              if (keycode < 0) {
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 5/6] qapi: generate list struct and visit_list for enum

[Michael Roth]

On Sat, Jun 02, 2012 at 06:54:27AM +0800, Amos Kong wrote:
> Currently, if define an 'enum' and use it in one command's data,
> List struct for enum could not be generated, but it's used in
> qmp function.
> 
> For example: KeyCodesList could not be generated.
> >>> qapi-schema.json:
> { 'enum': 'KeyCodes',
>   'data': [ 'shift', 'alt' ... ] }
> { 'command': 'sendkey',
>   'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> 
> >>> qmp-command.h:
> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> hold_time, Error **errp);
> 
> This patch makes qapi can generate List struct for enum.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>  scripts/qapi-visit.py |   14 +++++++++-----
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4a734f5..c9641fb 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -16,17 +16,36 @@ import os
>  import getopt
>  import errno
> 
> -def generate_fwd_struct(name, members):
> -    return mcgen('''
> +def generate_fwd_struct(name, members, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret += mcgen('''
>  typedef struct %(name)s %(name)s;
> 
> +''',
> +                     name=name)
> +    ret += mcgen('''
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +''',
> +                     name=name)
> +    if enum:
> +        ret += mcgen('''
> +         %(name)s value;
> +''',
> +                     name=name)
> +    else:
> +        ret += mcgen('''
> +         %(name)s * value;
> +''',
> +                     name=name)
> +
> +    ret += mcgen('''
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
>                   name=name)
> +    return ret
> 
>  def generate_struct(structname, fieldname, members):
>      ret = mcgen('''
> @@ -265,7 +284,8 @@ for expr in exprs:
>      if expr.has_key('type'):
>          ret += generate_fwd_struct(expr['type'], expr['data'])
>      elif expr.has_key('enum'):
> -        ret += generate_enum(expr['enum'], expr['data'])
> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>          fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>      elif expr.has_key('union'):
>          ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> @@ -289,6 +309,11 @@ for expr in exprs:
>          fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
>          ret += generate_type_cleanup_decl(expr['union'])
>          fdef.write(generate_type_cleanup(expr['union']) + "\n")
> +    elif expr.has_key('enum'):
> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")

This one is fine

> +        ret += generate_type_cleanup_decl(expr['enum'])
> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")

but we don't need this one since enum types are passed around by copy.

The qapi_free_X() functions are utility functions that aren't used by
the code generators, so there should be any harm in omitting these.


>      else:
>          continue
>      fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8d4e94a..e44edfa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,7 +81,7 @@ end:
>  ''')
>      return ret
> 
> -def generate_visit_list(name, members):
> +def generate_visit_list(name, members, enum=False):
>      return mcgen('''
> 

Were there plans to add a branch for enum=True? Otherwise we can drop this
from the patch.

>  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> @@ -160,12 +160,14 @@ end:
> 
>      return ret
> 
> -def generate_declaration(name, members, genlist=True):
> -    ret = mcgen('''
> +def generate_declaration(name, members, genlist=True, enum=False):
> +    ret = ""
> +    if not enum:
> +        ret = mcgen('''
> 
>  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
>  ''',
> -                name=name)
> +                    name=name)
> 
>      if genlist:
>          ret += mcgen('''
> @@ -293,10 +295,12 @@ for expr in exprs:
>          ret += generate_declaration(expr['union'], expr['data'])
>          fdecl.write(ret)
>      elif expr.has_key('enum'):
> -        ret = generate_visit_enum(expr['enum'], expr['data'])
> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>          fdef.write(ret)
> 
>          ret = generate_decl_enum(expr['enum'], expr['data'])
> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
>          fdecl.write(ret)
> 
>  fdecl.write('''
> -- 
> 1.7.1
> 
> 

Re: [Qemu-devel] [PATCH v2 5/6] qapi: generate list struct and visit_list for enum

[Michael Roth]

On Wed, Jun 06, 2012 at 03:40:37PM -0300, Luiz Capitulino wrote:
> On Sat,  2 Jun 2012 06:54:27 +0800
> Amos Kong <akong@redhat.com> wrote:
> 
> > Currently, if define an 'enum' and use it in one command's data,
> > List struct for enum could not be generated, but it's used in
> > qmp function.
> > 
> > For example: KeyCodesList could not be generated.
> > >>> qapi-schema.json:
> > { 'enum': 'KeyCodes',
> >   'data': [ 'shift', 'alt' ... ] }
> > { 'command': 'sendkey',
> >   'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> > 
> > >>> qmp-command.h:
> > void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> > hold_time, Error **errp);
> > 
> > This patch makes qapi can generate List struct for enum.
> 
> This patch does it the simple way, just like any type. It generates a enum list
> type and the functions qapi_free_yourenum() and qapi_free_yourenumlist().

Had a couple suggestions, but approach/patch seems reasonable to me.

> 
> The qapi_free_yourenum() list ends up doing nothing, so it could be a good
> idea to generate an empty body (also note that we're copying the argument's
> value, this could bite us in the future).

I think we can omit qapi_free_yourenum() completely. Humans will know
not to free non-allocated types, and the generated marshallers
don't use these interfaces.

> 
> Another point I was wondering is that, all enums will end up having the
> exact same code. So maybe we could generate a default int list and use it
> for all enums. Not sure it's worth it though.

Since it's generated code I don't think it's worth it, personally.

> 
> Michael, Paolo, ideas?
> 
> More review comments below.
> 
> 
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
> >  scripts/qapi-visit.py |   14 +++++++++-----
> >  2 files changed, 38 insertions(+), 9 deletions(-)
> > 
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 4a734f5..c9641fb 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -16,17 +16,36 @@ import os
> >  import getopt
> >  import errno
> >  
> > -def generate_fwd_struct(name, members):
> > -    return mcgen('''
> > +def generate_fwd_struct(name, members, enum=False):
> 
> I think it's better to have generate_fwd_enum_struct().
> 
> > +    ret = ""
> > +    if not enum:
> > +        ret += mcgen('''
> >  typedef struct %(name)s %(name)s;
> >  
> > +''',
> > +                     name=name)
> > +    ret += mcgen('''
> >  typedef struct %(name)sList
> >  {
> > -    %(name)s *value;
> > +''',
> > +                     name=name)
> > +    if enum:
> > +        ret += mcgen('''
> > +         %(name)s value;
> > +''',
> > +                     name=name)
> > +    else:
> > +        ret += mcgen('''
> > +         %(name)s * value;
> > +''',
> > +                     name=name)
> > +
> > +    ret += mcgen('''
> >      struct %(name)sList *next;
> >  } %(name)sList;
> >  ''',
> >                   name=name)
> > +    return ret
> >  
> >  def generate_struct(structname, fieldname, members):
> >      ret = mcgen('''
> > @@ -265,7 +284,8 @@ for expr in exprs:
> >      if expr.has_key('type'):
> >          ret += generate_fwd_struct(expr['type'], expr['data'])
> >      elif expr.has_key('enum'):
> > -        ret += generate_enum(expr['enum'], expr['data'])
> > +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
> 
> The new-line should be returned by generate_enum(). Same applies for the
> occurrences below.
> 
> > +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
> >          fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
> >      elif expr.has_key('union'):
> >          ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> > @@ -289,6 +309,11 @@ for expr in exprs:
> >          fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
> >          ret += generate_type_cleanup_decl(expr['union'])
> >          fdef.write(generate_type_cleanup(expr['union']) + "\n")
> > +    elif expr.has_key('enum'):
> > +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> > +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
> > +        ret += generate_type_cleanup_decl(expr['enum'])
> > +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
> >      else:
> >          continue
> >      fdecl.write(ret)
> > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> > index 8d4e94a..e44edfa 100644
> > --- a/scripts/qapi-visit.py
> > +++ b/scripts/qapi-visit.py
> > @@ -81,7 +81,7 @@ end:
> >  ''')
> >      return ret
> >  
> > -def generate_visit_list(name, members):
> > +def generate_visit_list(name, members, enum=False):
> >      return mcgen('''
> >  
> >  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> > @@ -160,12 +160,14 @@ end:
> >  
> >      return ret
> >  
> > -def generate_declaration(name, members, genlist=True):
> > -    ret = mcgen('''
> > +def generate_declaration(name, members, genlist=True, enum=False):
> > +    ret = ""
> > +    if not enum:
> > +        ret = mcgen('''
> >  
> >  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
> >  ''',
> > -                name=name)
> > +                    name=name)
> 
> Why this change?
> 
> >  
> >      if genlist:
> >          ret += mcgen('''
> > @@ -293,10 +295,12 @@ for expr in exprs:
> >          ret += generate_declaration(expr['union'], expr['data'])
> >          fdecl.write(ret)
> >      elif expr.has_key('enum'):
> > -        ret = generate_visit_enum(expr['enum'], expr['data'])
> > +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> > +        ret += generate_visit_enum(expr['enum'], expr['data'])
> >          fdef.write(ret)
> >  
> >          ret = generate_decl_enum(expr['enum'], expr['data'])
> > +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
> >          fdecl.write(ret)
> >  
> >  fdecl.write('''
> 
> 

Re: [Qemu-devel] [PATCH v2 5/6] qapi: generate list struct and visit_list for enum

[Amos Kong]

On 07/06/12 08:26, Michael Roth wrote:
> On Wed, Jun 06, 2012 at 03:40:37PM -0300, Luiz Capitulino wrote:
>> On Sat,  2 Jun 2012 06:54:27 +0800
>> Amos Kong<akong@redhat.com>  wrote:
>>
>>> Currently, if define an 'enum' and use it in one command's data,
>>> List struct for enum could not be generated, but it's used in
>>> qmp function.
>>>
>>> For example: KeyCodesList could not be generated.
>>>>>> qapi-schema.json:
>>> { 'enum': 'KeyCodes',
>>>    'data': [ 'shift', 'alt' ... ] }
>>> { 'command': 'sendkey',
>>>    'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
>>>
>>>>>> qmp-command.h:
>>> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
>>> hold_time, Error **errp);
>>>
>>> This patch makes qapi can generate List struct for enum.
>>
>> This patch does it the simple way, just like any type. It generates a enum list
>> type and the functions qapi_free_yourenum() and qapi_free_yourenumlist().
>
> Had a couple suggestions, but approach/patch seems reasonable to me.
>
>>
>> The qapi_free_yourenum() list ends up doing nothing, so it could be a good
>> idea to generate an empty body (also note that we're copying the argument's
>> value, this could bite us in the future).
>
> I think we can omit qapi_free_yourenum() completely. Humans will know
> not to free non-allocated types, and the generated marshallers
> don't use these interfaces.
>
>>
>> Another point I was wondering is that, all enums will end up having the
>> exact same code. So maybe we could generate a default int list and use it
>> for all enums. Not sure it's worth it though.
>
> Since it's generated code I don't think it's worth it, personally.
>
>>
>> Michael, Paolo, ideas?
>>
>> More review comments below.


Thanks for your review!

>>> Signed-off-by: Amos Kong<akong@redhat.com>
>>> ---
>>>   scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>>>   scripts/qapi-visit.py |   14 +++++++++-----
>>>   2 files changed, 38 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>>> index 4a734f5..c9641fb 100644
>>> --- a/scripts/qapi-types.py
>>> +++ b/scripts/qapi-types.py
>>> @@ -16,17 +16,36 @@ import os
>>>   import getopt
>>>   import errno
>>>
>>> -def generate_fwd_struct(name, members):
>>> -    return mcgen('''
>>> +def generate_fwd_struct(name, members, enum=False):
>>
>> I think it's better to have generate_fwd_enum_struct().

I tried this before, too many duplicate codes would be introduced,
but it's more clear.

two functions need to be added
   qapi-types.py: def generate_fwd_enum_struct(name, members):
   qapi-visit.py: def generate_enum_declaration(name, members, 
genlist=True):

>>> +    ret = ""
>>> +    if not enum:
>>> +        ret += mcgen('''
>>>   typedef struct %(name)s %(name)s;
>>>
>>> +''',
>>> +                     name=name)
>>> +    ret += mcgen('''
>>>   typedef struct %(name)sList
>>>   {
>>> -    %(name)s *value;
>>> +''',
>>> +                     name=name)
>>> +    if enum:
>>> +        ret += mcgen('''
>>> +         %(name)s value;
>>> +''',
>>> +                     name=name)
>>> +    else:
>>> +        ret += mcgen('''
>>> +         %(name)s * value;
>>> +''',
>>> +                     name=name)
>>> +
>>> +    ret += mcgen('''
>>>       struct %(name)sList *next;
>>>   } %(name)sList;
>>>   ''',
>>>                    name=name)
>>> +    return ret
>>>
>>>   def generate_struct(structname, fieldname, members):
>>>       ret = mcgen('''
>>> @@ -265,7 +284,8 @@ for expr in exprs:
>>>       if expr.has_key('type'):
>>>           ret += generate_fwd_struct(expr['type'], expr['data'])
>>>       elif expr.has_key('enum'):
>>> -        ret += generate_enum(expr['enum'], expr['data'])
>>> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
>>
>> The new-line should be returned by generate_enum(). Same applies for the
>> occurrences below.


"\n" is used to add a blank line here.

>>> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>>>           fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>>>       elif expr.has_key('union'):
>>>           ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
>>> @@ -289,6 +309,11 @@ for expr in exprs:
>>>           fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
>>>           ret += generate_type_cleanup_decl(expr['union'])
>>>           fdef.write(generate_type_cleanup(expr['union']) + "\n")
>>> +    elif expr.has_key('enum'):
>>> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
>>> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
>>> +        ret += generate_type_cleanup_decl(expr['enum'])
>>> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
>>>       else:
>>>           continue
>>>       fdecl.write(ret)
>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>> index 8d4e94a..e44edfa 100644
>>> --- a/scripts/qapi-visit.py
>>> +++ b/scripts/qapi-visit.py
>>> @@ -81,7 +81,7 @@ end:
>>>   ''')
>>>       return ret
>>>
>>> -def generate_visit_list(name, members):
>>> +def generate_visit_list(name, members, enum=False):
>>>       return mcgen('''
>>>
>>>   void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
>>> @@ -160,12 +160,14 @@ end:
>>>
>>>       return ret
>>>
>>> -def generate_declaration(name, members, genlist=True):
>>> -    ret = mcgen('''
>>> +def generate_declaration(name, members, genlist=True, enum=False):
>>> +    ret = ""
>>> +    if not enum:
>>> +        ret = mcgen('''

>>>   void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
>>>   ''',
>>> -                name=name)
>>> +                    name=name)
>>
>> Why this change?


Indent of above "mcgen(" changes, make them aligned.

>>>
>>>       if genlist:
>>>           ret += mcgen('''
>>> @@ -293,10 +295,12 @@ for expr in exprs:
>>>           ret += generate_declaration(expr['union'], expr['data'])
>>>           fdecl.write(ret)
>>>       elif expr.has_key('enum'):
>>> -        ret = generate_visit_enum(expr['enum'], expr['data'])
>>> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
>>> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>>>           fdef.write(ret)
>>>
>>>           ret = generate_decl_enum(expr['enum'], expr['data'])
>>> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
>>>           fdecl.write(ret)
>>>
>>>   fdecl.write('''
>>
>>

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 5/6] qapi: generate list struct and visit_list for enum

[Amos Kong]

On 07/06/12 08:15, Michael Roth wrote:
> On Sat, Jun 02, 2012 at 06:54:27AM +0800, Amos Kong wrote:
>> Currently, if define an 'enum' and use it in one command's data,
>> List struct for enum could not be generated, but it's used in
>> qmp function.
>>
>> For example: KeyCodesList could not be generated.
>>>>> qapi-schema.json:
>> { 'enum': 'KeyCodes',
>>    'data': [ 'shift', 'alt' ... ] }
>> { 'command': 'sendkey',
>>    'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
>>
>>>>> qmp-command.h:
>> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
>> hold_time, Error **errp);
>>
>> This patch makes qapi can generate List struct for enum.
>>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
>>   scripts/qapi-visit.py |   14 +++++++++-----
>>   2 files changed, 38 insertions(+), 9 deletions(-)
>>
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index 4a734f5..c9641fb 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -16,17 +16,36 @@ import os
>>   import getopt
>>   import errno
>>
>> -def generate_fwd_struct(name, members):
>> -    return mcgen('''
>> +def generate_fwd_struct(name, members, enum=False):
>> +    ret = ""
>> +    if not enum:
>> +        ret += mcgen('''
>>   typedef struct %(name)s %(name)s;
>>
>> +''',
>> +                     name=name)
>> +    ret += mcgen('''
>>   typedef struct %(name)sList
>>   {
>> -    %(name)s *value;
>> +''',
>> +                     name=name)
>> +    if enum:
>> +        ret += mcgen('''
>> +         %(name)s value;
>> +''',
>> +                     name=name)
>> +    else:
>> +        ret += mcgen('''
>> +         %(name)s * value;
>> +''',
>> +                     name=name)
>> +
>> +    ret += mcgen('''
>>       struct %(name)sList *next;
>>   } %(name)sList;
>>   ''',
>>                    name=name)
>> +    return ret
>>
>>   def generate_struct(structname, fieldname, members):
>>       ret = mcgen('''
>> @@ -265,7 +284,8 @@ for expr in exprs:
>>       if expr.has_key('type'):
>>           ret += generate_fwd_struct(expr['type'], expr['data'])
>>       elif expr.has_key('enum'):
>> -        ret += generate_enum(expr['enum'], expr['data'])
>> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
>> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
>>           fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>>       elif expr.has_key('union'):
>>           ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
>> @@ -289,6 +309,11 @@ for expr in exprs:
>>           fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
>>           ret += generate_type_cleanup_decl(expr['union'])
>>           fdef.write(generate_type_cleanup(expr['union']) + "\n")
>> +    elif expr.has_key('enum'):
>> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
>> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
>
> This one is fine
>
>> +        ret += generate_type_cleanup_decl(expr['enum'])
>> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
>
> but we don't need this one since enum types are passed around by copy.
>
> The qapi_free_X() functions are utility functions that aren't used by
> the code generators, so there should be any harm in omitting these.

Ok, will drop those two sentences.


>>       else:
>>           continue
>>       fdecl.write(ret)
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 8d4e94a..e44edfa 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -81,7 +81,7 @@ end:
>>   ''')
>>       return ret
>>
>> -def generate_visit_list(name, members):
>> +def generate_visit_list(name, members, enum=False):
>>       return mcgen('''
>>
>
> Were there plans to add a branch for enum=True? Otherwise we can drop this
> from the patch.


redundant, will drop it.

>>   void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
>> @@ -160,12 +160,14 @@ end:
>>
>>       return ret
>>
>> -def generate_declaration(name, members, genlist=True):
>> -    ret = mcgen('''
>> +def generate_declaration(name, members, genlist=True, enum=False):
>> +    ret = ""
>> +    if not enum:
>> +        ret = mcgen('''
>>
>>   void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
>>   ''',
>> -                name=name)
>> +                    name=name)
>>
>>       if genlist:
>>           ret += mcgen('''
>> @@ -293,10 +295,12 @@ for expr in exprs:
>>           ret += generate_declaration(expr['union'], expr['data'])
>>           fdecl.write(ret)
>>       elif expr.has_key('enum'):
>> -        ret = generate_visit_enum(expr['enum'], expr['data'])
>> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
>> +        ret += generate_visit_enum(expr['enum'], expr['data'])
>>           fdef.write(ret)
>>
>>           ret = generate_decl_enum(expr['enum'], expr['data'])
>> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
>>           fdecl.write(ret)
>>
>>   fdecl.write('''
>> --
>> 1.7.1
>>
>>

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 6/6] qapi: convert sendkey

[Anthony Liguori]

On 06/06/2012 07:58 PM, Eric Blake wrote:
> On 06/06/2012 01:13 AM, Amos Kong wrote:
>
>>>> Ho, I found another bug in my code, key in decimal or hexadecimal
>>>> format is not supported. I will fix it.
>>>
>>> How do you plan to support decimal in QMP?
>>
>> In old do_sendkey(), only key-name and hexadecimal were supported,
>> the description needs to be fixed.
>>
>
>>
>> It seems we can only support key-name for QMP, and support
>> key-name/hexadecimal
>> for HMP. Is it acceptable?
>
> Yes, that was my argument for bare minimum support - anyone using QMP
> (like libvirt) will have to be smart enough to use key-name only, while
> anyone using HMP will be able to use hex because HMP will convert hex to
> key-name at the appropriate time before calling into QMP.
>
> Of course, being able to support hex from QMP would be a nice feature,
> but I'm not sure how to add it in.

Why is it a nice feature?

I don't quite understand why anyone would want to use hex actually...  You would 
have to convert from whatever symbolic code you're using to QEMU's hex value so 
why not just stay in symbolic representation.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH v2 6/6] qapi: convert sendkey

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

On 06/06/2012 10:51 PM, Anthony Liguori wrote:

>> Of course, being able to support hex from QMP would be a nice feature,
>> but I'm not sure how to add it in.
> 
> Why is it a nice feature?

Because libvirt already allows users to pass in integers instead of
symbolic names.  Providing integer support in QMP would let the path be
passthrough, instead of libvirt doing a reverse lookup to convert
integer to symbol just to call QMP for qemu to do a lookup from symbol
back to integer.

> 
> I don't quite understand why anyone would want to use hex actually... 
> You would have to convert from whatever symbolic code you're using to
> QEMU's hex value so why not just stay in symbolic representation.

Staying just symbolic is fine - remember, I already stated that libvirt
already has a lookup table to convert between several different keycode
sets already, and Dan Berrange has already proposed sharing that table
with qemu.  And settling on symbolic as the only interface, even if it
is less efficient than integer passthrough, is at least easier to maintain.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] qapi: generate list struct and visit_list for enum

[Luiz Capitulino]

On Thu, 07 Jun 2012 10:52:46 +0800
Amos Kong <akong@redhat.com> wrote:

> On 07/06/12 08:26, Michael Roth wrote:
> > On Wed, Jun 06, 2012 at 03:40:37PM -0300, Luiz Capitulino wrote:
> >> On Sat,  2 Jun 2012 06:54:27 +0800
> >> Amos Kong<akong@redhat.com>  wrote:
> >>
> >>> Currently, if define an 'enum' and use it in one command's data,
> >>> List struct for enum could not be generated, but it's used in
> >>> qmp function.
> >>>
> >>> For example: KeyCodesList could not be generated.
> >>>>>> qapi-schema.json:
> >>> { 'enum': 'KeyCodes',
> >>>    'data': [ 'shift', 'alt' ... ] }
> >>> { 'command': 'sendkey',
> >>>    'data': { 'keys': ['KeyCodes'], '*hold-time': 'int' } }
> >>>
> >>>>>> qmp-command.h:
> >>> void qmp_sendkey(KeyCodesList * keys, bool has_hold_time, int64_t
> >>> hold_time, Error **errp);
> >>>
> >>> This patch makes qapi can generate List struct for enum.
> >>
> >> This patch does it the simple way, just like any type. It generates a enum list
> >> type and the functions qapi_free_yourenum() and qapi_free_yourenumlist().
> >
> > Had a couple suggestions, but approach/patch seems reasonable to me.
> >
> >>
> >> The qapi_free_yourenum() list ends up doing nothing, so it could be a good
> >> idea to generate an empty body (also note that we're copying the argument's
> >> value, this could bite us in the future).
> >
> > I think we can omit qapi_free_yourenum() completely. Humans will know
> > not to free non-allocated types, and the generated marshallers
> > don't use these interfaces.
> >
> >>
> >> Another point I was wondering is that, all enums will end up having the
> >> exact same code. So maybe we could generate a default int list and use it
> >> for all enums. Not sure it's worth it though.
> >
> > Since it's generated code I don't think it's worth it, personally.
> >
> >>
> >> Michael, Paolo, ideas?
> >>
> >> More review comments below.
> 
> 
> Thanks for your review!
> 
> >>> Signed-off-by: Amos Kong<akong@redhat.com>
> >>> ---
> >>>   scripts/qapi-types.py |   33 +++++++++++++++++++++++++++++----
> >>>   scripts/qapi-visit.py |   14 +++++++++-----
> >>>   2 files changed, 38 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> >>> index 4a734f5..c9641fb 100644
> >>> --- a/scripts/qapi-types.py
> >>> +++ b/scripts/qapi-types.py
> >>> @@ -16,17 +16,36 @@ import os
> >>>   import getopt
> >>>   import errno
> >>>
> >>> -def generate_fwd_struct(name, members):
> >>> -    return mcgen('''
> >>> +def generate_fwd_struct(name, members, enum=False):
> >>
> >> I think it's better to have generate_fwd_enum_struct().
> 
> I tried this before, too many duplicate codes would be introduced,
> but it's more clear.

You could try to move the duplicated code to a third function (or move
the specific bits to generate_fwd_enum_struct() and generate_enum_declaration()).

> 
> two functions need to be added
>    qapi-types.py: def generate_fwd_enum_struct(name, members):
>    qapi-visit.py: def generate_enum_declaration(name, members, 
> genlist=True):
> 
> >>> +    ret = ""
> >>> +    if not enum:
> >>> +        ret += mcgen('''
> >>>   typedef struct %(name)s %(name)s;
> >>>
> >>> +''',
> >>> +                     name=name)
> >>> +    ret += mcgen('''
> >>>   typedef struct %(name)sList
> >>>   {
> >>> -    %(name)s *value;
> >>> +''',
> >>> +                     name=name)
> >>> +    if enum:
> >>> +        ret += mcgen('''
> >>> +         %(name)s value;
> >>> +''',
> >>> +                     name=name)
> >>> +    else:
> >>> +        ret += mcgen('''
> >>> +         %(name)s * value;
> >>> +''',
> >>> +                     name=name)
> >>> +
> >>> +    ret += mcgen('''
> >>>       struct %(name)sList *next;
> >>>   } %(name)sList;
> >>>   ''',
> >>>                    name=name)
> >>> +    return ret
> >>>
> >>>   def generate_struct(structname, fieldname, members):
> >>>       ret = mcgen('''
> >>> @@ -265,7 +284,8 @@ for expr in exprs:
> >>>       if expr.has_key('type'):
> >>>           ret += generate_fwd_struct(expr['type'], expr['data'])
> >>>       elif expr.has_key('enum'):
> >>> -        ret += generate_enum(expr['enum'], expr['data'])
> >>> +        ret += generate_enum(expr['enum'], expr['data']) + "\n"
> >>
> >> The new-line should be returned by generate_enum(). Same applies for the
> >> occurrences below.
> 
> 
> "\n" is used to add a blank line here.
> 
> >>> +        ret += generate_fwd_struct(expr['enum'], expr['data'], True)
> >>>           fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
> >>>       elif expr.has_key('union'):
> >>>           ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> >>> @@ -289,6 +309,11 @@ for expr in exprs:
> >>>           fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
> >>>           ret += generate_type_cleanup_decl(expr['union'])
> >>>           fdef.write(generate_type_cleanup(expr['union']) + "\n")
> >>> +    elif expr.has_key('enum'):
> >>> +        ret += generate_type_cleanup_decl(expr['enum'] + "List")
> >>> +        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
> >>> +        ret += generate_type_cleanup_decl(expr['enum'])
> >>> +        fdef.write(generate_type_cleanup(expr['enum']) + "\n")
> >>>       else:
> >>>           continue
> >>>       fdecl.write(ret)
> >>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> >>> index 8d4e94a..e44edfa 100644
> >>> --- a/scripts/qapi-visit.py
> >>> +++ b/scripts/qapi-visit.py
> >>> @@ -81,7 +81,7 @@ end:
> >>>   ''')
> >>>       return ret
> >>>
> >>> -def generate_visit_list(name, members):
> >>> +def generate_visit_list(name, members, enum=False):
> >>>       return mcgen('''
> >>>
> >>>   void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> >>> @@ -160,12 +160,14 @@ end:
> >>>
> >>>       return ret
> >>>
> >>> -def generate_declaration(name, members, genlist=True):
> >>> -    ret = mcgen('''
> >>> +def generate_declaration(name, members, genlist=True, enum=False):
> >>> +    ret = ""
> >>> +    if not enum:
> >>> +        ret = mcgen('''
> 
> >>>   void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
> >>>   ''',
> >>> -                name=name)
> >>> +                    name=name)
> >>
> >> Why this change?
> 
> 
> Indent of above "mcgen(" changes, make them aligned.
> 
> >>>
> >>>       if genlist:
> >>>           ret += mcgen('''
> >>> @@ -293,10 +295,12 @@ for expr in exprs:
> >>>           ret += generate_declaration(expr['union'], expr['data'])
> >>>           fdecl.write(ret)
> >>>       elif expr.has_key('enum'):
> >>> -        ret = generate_visit_enum(expr['enum'], expr['data'])
> >>> +        ret = generate_visit_list(expr['enum'], expr['data'], True)
> >>> +        ret += generate_visit_enum(expr['enum'], expr['data'])
> >>>           fdef.write(ret)
> >>>
> >>>           ret = generate_decl_enum(expr['enum'], expr['data'])
> >>> +        ret += generate_declaration(expr['enum'], expr['data'], enum=True)
> >>>           fdecl.write(ret)
> >>>
> >>>   fdecl.write('''
> >>
> >>
> 

Re: [Qemu-devel] [PATCH v2 1/6] qerror: add MAX_KEYCODES 16

[Amos Kong]

On 12/06/12 01:25, Luiz Capitulino wrote:
>>

Hi Luiz, Anthony

>>  BTW, why is there a 16 keycode limit?

'Sendkey' command was added by this commit 
a3a91a355bc6107b7d06d722fb97d2b80065ee0b
Limitation of keycodes number (16) was also introduced here,
and I didn't find the root reason.

>>
> That's a good point. This comes form the array used by the original
> implementation to store the keycodes.
>
> Amos, is it still needed now that we're using qapi lists?


I try to drop this limitation from monitor.c [1], then execute

(qemu) sendkey 1-2-3-4-5-6-7-8-9-0-1-2-3-4-5-6-7-8-9-0

kbd_put_keycode() are called for all (20) keycodes,
but only '123456789012345' can be sent to guest.


It seems we need to notice user when inputted keys are more than 16.


[1] 
https://github.com/kongove/QEMU/commit/a198cdcce3ce4c1632221ac7f1e7c4e8efcd9c82



-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/6] qerror: add MAX_KEYCODES 16

[Amos Kong]

On 14/06/12 18:20, Amos Kong wrote:
> On 12/06/12 01:25, Luiz Capitulino wrote:
>>>
>
> Hi Luiz, Anthony
>
>>> BTW, why is there a 16 keycode limit?
>
> 'Sendkey' command was added by this commit
> a3a91a355bc6107b7d06d722fb97d2b80065ee0b
> Limitation of keycodes number (16) was also introduced here,
> and I didn't find the root reason.
>
>>>
>> That's a good point. This comes form the array used by the original
>> implementation to store the keycodes.
>>
>> Amos, is it still needed now that we're using qapi lists?
>
>
> I try to drop this limitation from monitor.c [1], then execute
>
> (qemu) sendkey 1-2-3-4-5-6-7-8-9-0-1-2-3-4-5-6-7-8-9-0
>
> kbd_put_keycode() are called for all (20) keycodes,
> but only '123456789012345' can be sent to guest.
>
>
> It seems we need to notice user when inputted keys are more than 16.


Hi Gerd,

When I use 'sendkey' command to send key-series to guest, some keyboard
events will be send. There is a limitation (16) that was introduced by this
old commit c8256f9d (without description). Do you know the reason?


I found in bonzini's commit 13f8b97a :
+#define QUEUE_LENGTH    16 /* should be enough for a triple-click */
bonzini told me that the reason to do it for the keyboard, was
to do ctrl-alt-delete, nothing more

I drop this limitation (16) in monitor.c for sendkey command,
when I execute (qemu) sendkey 1-2-3-4-5-6-7-8-9-0-1-2-3-4-5-6-7-8-9-0 ,
hw/ps2.c:ps2_put_keycode() is called for all keys,
but only '123456789012345' is inputted to guest.

There is a 'PS2_QUEUE_SIZE' (256) in hw/ps2.c, this event queue is
shared by mouse and keyboard, but I only inputted 20 keys.

I didn't find where the other keys are ignored, and there is
no warning in stderr.

Hi Anthony, Luiz,

However, the limitation (16) for sendkey is necessary, 16 is enough for 
all combination keys


> [1]
> https://github.com/kongove/QEMU/commit/a198cdcce3ce4c1632221ac7f1e7c4e8efcd9c82

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/6] qerror: add MAX_KEYCODES 16

[Gerd Hoffmann]

  Hi,

>> It seems we need to notice user when inputted keys are more than 16.
> 
> Hi Gerd,
> 
> When I use 'sendkey' command to send key-series to guest, some keyboard
> events will be send. There is a limitation (16) that was introduced by this
> old commit c8256f9d (without description). Do you know the reason?

Probably hardware limitation, ps/2 keyboards can buffer up to 16 keys IIRC.

Likewise the usb hid devices can buffer up to 16 events.  In that case
it is just a qemu implementation detail and not a property of the
hardware we are emulating, so it can be changed.  Not trivially though
as the buffer is part of the migration data, so it is more work that
just changing a #define.

HTH,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/6] qerror: add MAX_KEYCODES 16

[Luiz Capitulino]

On Fri, 15 Jun 2012 09:57:49 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> >> It seems we need to notice user when inputted keys are more than 16.
> > 
> > Hi Gerd,
> > 
> > When I use 'sendkey' command to send key-series to guest, some keyboard
> > events will be send. There is a limitation (16) that was introduced by this
> > old commit c8256f9d (without description). Do you know the reason?
> 
> Probably hardware limitation, ps/2 keyboards can buffer up to 16 keys IIRC.

Then the perfect thing to do would be to drop the MAX_KEYCODES check from
the sendkey command and move bounds checking down to the device emulation code.

However, this will require a bit of code churn if we do it for all devices,
and won't buy us much, as the most likely reason for the error is a client/user
trying to send too many keys in parallel to the guest, right?

If this is right, then I think that the best thing to do would be to drop the
MAX_KEYCODES check from the sendkey command and document that devices can drop
keys if too many of them are sent in parallel or too fast (we can mention ps/2
as an example of a 16 bytes limit).

> 
> Likewise the usb hid devices can buffer up to 16 events.  In that case
> it is just a qemu implementation detail and not a property of the
> hardware we are emulating, so it can be changed.  Not trivially though
> as the buffer is part of the migration data, so it is more work that
> just changing a #define.

Re: [Qemu-devel] [PATCH v2 1/6] qerror: add MAX_KEYCODES 16

[Amos Kong]

On 06/15/2012 09:35 PM, Luiz Capitulino wrote:
> On Fri, 15 Jun 2012 09:57:49 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>>   Hi,
>>
>>>> It seems we need to notice user when inputted keys are more than 16.
>>>
>>> Hi Gerd,
>>>
>>> When I use 'sendkey' command to send key-series to guest, some keyboard
>>> events will be send. There is a limitation (16) that was introduced by this
>>> old commit c8256f9d (without description). Do you know the reason?
>>
>> Probably hardware limitation, ps/2 keyboards can buffer up to 16 keys IIRC.
> 
> Then the perfect thing to do would be to drop the MAX_KEYCODES check from
> the sendkey command and move bounds checking down to the device emulation code.
>
>
> However, this will require a bit of code churn if we do it for all devices,
> and won't buy us much, as the most likely reason for the error is a client/user
> trying to send too many keys in parallel to the guest, right?

Agree, we can notice in stderr when the redundant keys are ignored as hid.


#define QUEUE_LENGTH    16 /* should be enough for a triple-click */

static void hid_keyboard_event(void *opaque, int keycode)
{
    ...
    if (hs->n == QUEUE_LENGTH) {
        fprintf(stderr, "usb-kbd: warning: key event queue full\n");
        return;
    }


> If this is right, then I think that the best thing to do would be to drop the
> MAX_KEYCODES check from the sendkey command and document that devices can drop
> keys if too many of them are sent in parallel or too fast (we can mention ps/2
> as an example of a 16 bytes limit).
> 
>>
>> Likewise the usb hid devices can buffer up to 16 events.  In that case
>> it is just a qemu implementation detail and not a property of the
>> hardware we are emulating, so it can be changed.  Not trivially though
>> as the buffer is part of the migration data, so it is more work that
>> just changing a #define.


-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/6] qerror: add MAX_KEYCODES 16

[Amos Kong]

On 18/06/12 23:30, Amos Kong wrote:
> On 06/15/2012 09:35 PM, Luiz Capitulino wrote:
>> On Fri, 15 Jun 2012 09:57:49 +0200
>> Gerd Hoffmann<kraxel@redhat.com>  wrote:
>>
>>>    Hi,
>>>
>>>>> It seems we need to notice user when inputted keys are more than 16.
>>>>
>>>> Hi Gerd,
>>>>
>>>> When I use 'sendkey' command to send key-series to guest, some keyboard
>>>> events will be send. There is a limitation (16) that was introduced by this
>>>> old commit c8256f9d (without description). Do you know the reason?
>>>
>>> Probably hardware limitation, ps/2 keyboards can buffer up to 16 keys IIRC.
>>
>> Then the perfect thing to do would be to drop the MAX_KEYCODES check from
>> the sendkey command and move bounds checking down to the device emulation code.
>>
>>
>> However, this will require a bit of code churn if we do it for all devices,
>> and won't buy us much, as the most likely reason for the error is a client/user
>> trying to send too many keys in parallel to the guest, right?
>
> Agree, we can notice in stderr when the redundant keys are ignored as hid.
>
>
> #define QUEUE_LENGTH    16 /* should be enough for a triple-click */
>
> static void hid_keyboard_event(void *opaque, int keycode)
> {
>      ...
>      if (hs->n == QUEUE_LENGTH) {
>          fprintf(stderr, "usb-kbd: warning: key event queue full\n");
>          return;
>      }


I dropped the limitation in sendkey command,
and didn't change current ps2.c, executed some
tests in different environments.

environment             max inputted key number
------------            -----------------------
win7 notepad            100
rhel6 grub              15
rhel6 pxe               15
rhel6 login window      10
rhel6 vim               16
rhel6 terminal(init 3)  200


It seems original 256 queue limitation in ps2.c is fine.
I would only drop limitation(16) in old sendkey command,
it's secure.


>> If this is right, then I think that the best thing to do would be to drop the
>> MAX_KEYCODES check from the sendkey command and document that devices can drop
>> keys if too many of them are sent in parallel or too fast (we can mention ps/2
>> as an example of a 16 bytes limit).
>>
>>>
>>> Likewise the usb hid devices can buffer up to 16 events.  In that case
>>> it is just a qemu implementation detail and not a property of the
>>> hardware we are emulating, so it can be changed.  Not trivially though
>>> as the buffer is part of the migration data, so it is more work that
>>> just changing a #define.


-- 
			Amos.