Qemu support for multiple keyboard devices - v2

Currently you get segfault when trying to remove keyboard (device_del monitor command) because no keyboard handling is done.

This patch add QEMUPutKbdEntry structure, handling each keyboard entry. Adding a keyboard add to the list, removing keyboard select the previous keyboard in list.

Signed-off-by Shahar Havivi <[email protected]>

---
 console.h            |   12 ++++++-
 hw/adb.c             |    2 +-
 hw/escc.c            |    3 +-
 hw/musicpal.c        |    2 +-
 hw/nseries.c         |    4 +-
 hw/palm.c            |    2 +-
 hw/ps2.c             |    2 +-
 hw/pxa2xx_keypad.c   |    2 +-
 hw/spitz.c           |    2 +-
 hw/stellaris_input.c |    2 +-
 hw/syborg_keyboard.c |    2 +-
 hw/usb-hid.c         |   10 ++++--
 hw/xenfb.c           |    4 +-
 input.c              |   90 ++++++++++++++++++++++++++++++++++++++++++++-----
 14 files changed, 112 insertions(+), 27 deletions(-)

console.h +11 -1

index 6def115..16c9c3d 100644
@@ -41,7 +41,17 @@ typedef struct QEMUPutLEDEntry {
     QTAILQ_ENTRY(QEMUPutLEDEntry) next;
 } QEMUPutLEDEntry;
 
-void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
+typedef struct QEMUPutKbdEntry {
+    char *qemu_put_kbd_name;
+    QEMUPutKBDEvent *qemu_put_kbd_event;
+    void *qemu_put_kbd_event_opaque;
+    struct QEMUPutKbdEntry *next;
+} QEMUPutKbdEntry;
+
+QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func,
+                                            void *opaque,
+                                            const char *name);
+void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry);
 QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name);

input.c +8 -8

index 8f0941e..563ecad 100644
@@ -28,20 +28,14 @@
 #include "console.h"
 #include "qjson.h"
 
-static QEMUPutKBDEvent *qemu_put_kbd_event;
-static void *qemu_put_kbd_event_opaque;
+static QEMUPutKbdEntry *qemu_put_kbd_event_head;
+static QEMUPutKbdEntry *qemu_put_kbd_event_current;
 static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers);
 static QTAILQ_HEAD(, QEMUPutMouseEntry) mouse_handlers =
     QTAILQ_HEAD_INITIALIZER(mouse_handlers);
 static NotifierList mouse_mode_notifiers = 
     NOTIFIER_LIST_INITIALIZER(mouse_mode_notifiers);
 
-void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
-{
-    qemu_put_kbd_event_opaque = opaque;
-    qemu_put_kbd_event = func;
-}
-
 static void check_mode_change(void)
 {
     static int current_is_absolute, current_has_absolute;

@@ -60,6 +54,81 @@ static void check_mode_change(void)
     current_has_absolute = has_absolute;
 }
 
+QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func,
+                                            void *opaque,
+                                            const char *name)
+{
+    QEMUPutKbdEntry *s, *cursor;
+

input.c +47 -0

+
+    s = qemu_mallocz(sizeof(QEMUPutKbdEntry));
+
+    s->qemu_put_kbd_event_opaque = opaque;
+    s->qemu_put_kbd_event = func;
+    s->qemu_put_kbd_name = qemu_strdup(name);
+    s->next = NULL;
+
+    if (!qemu_put_kbd_event_head) {
+        qemu_put_kbd_event_head = s;
+        qemu_put_kbd_event_current = s;
+        return s;
+    }
+
+    cursor = qemu_put_kbd_event_head;
+    while (cursor->next) {
+        cursor = cursor->next;
+    }
+
+    cursor->next = s;
+    qemu_put_kbd_event_current = s;
+
+    return s;
+}
+
+void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry)
+{
+    QEMUPutKbdEntry *prev = NULL, *cursor;
+
+    if (!qemu_put_kbd_event_head || !entry) {
+        return;
+    }
+
+    cursor = qemu_put_kbd_event_head;
+    while (cursor && cursor != entry) {
+        prev = cursor;
+        cursor = cursor->next;
+    }
+
+    if (cursor == NULL) {
+        return;
+    } else if (prev == NULL) {
+        qemu_put_kbd_event_head = cursor->next;
+        if (qemu_put_kbd_event_current == entry) {
+            qemu_put_kbd_event_current = cursor->next;
+        }
+        qemu_free(entry);

input.c +5 -2

+}
+
 QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name)

@@ -123,8 +192,9 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
 
 void kbd_put_keycode(int keycode)
 {
-    if (qemu_put_kbd_event) {
-        qemu_put_kbd_event(qemu_put_kbd_event_opaque, keycode);
+    if (qemu_put_kbd_event_current) {
+        qemu_put_kbd_event_current->qemu_put_kbd_event(
+            qemu_put_kbd_event_current->qemu_put_kbd_event_opaque, keycode);
     }
 }

Quoted 11 lines

(quoted diff) +11 -0

+QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func,
+                                            void *opaque,
+                                            const char *name)
+{
+    QEMUPutKbdEntry *s, *cursor;
+
+    cursor = qemu_put_kbd_event_head;
+    while (cursor) {
+        if (cursor->qemu_put_kbd_event == func &&
+            cursor->qemu_put_kbd_event_opaque == opaque) {
+

Review

Quoted 5 lines

(quoted diff) +5 -0

+            qemu_put_kbd_event_current = cursor;
+            return cursor;
+        }
+        cursor = cursor->next;
+    }

You're chasing list pointers. Why not use a suitable list type from qemu-queue.h?

Two new monitor commands: adding ability to handle which keyboard qemu will use and to see which keyboard are currently available.

$ info keyboard
$ keyboard_set <index>

Signed-off-by Shahar Havivi <[email protected]>

---
 console.h       |    4 ++
 input.c         |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 monitor.c       |    8 ++++
 qemu-monitor.hx |   17 +++++++++
 qerror.c        |    8 ++++
 5 files changed, 141 insertions(+), 0 deletions(-)

console.h +4 -0

index 16c9c3d..7efa88e 100644
@@ -85,6 +85,10 @@ void do_info_mice_print(Monitor *mon, const QObject *data);
 void do_info_mice(Monitor *mon, QObject **ret_data);
 void do_mouse_set(Monitor *mon, const QDict *qdict);
 
+void do_info_keyboard_print(Monitor *mon, const QObject *data);
+void do_info_keyboard(Monitor *mon, QObject **ret_data);
+int do_keyboard_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
+
 /* keysym is a unicode code except for special keys (see QEMU_KEY_xxx
    constants) */
 #define QEMU_KEY_ESC1(c) ((c) | 0xe100)

input.c +57 -0

index 563ecad..4daaeb0 100644
@@ -368,3 +368,107 @@ void qemu_remove_mouse_mode_change_notifier(Notifier *notify)
 {
     notifier_list_remove(&mouse_mode_notifiers, notify);
 }
+
+static void info_keyboard_iter(QObject *data, void *opaque)
+{
+    QDict *kbd;
+    Monitor *mon = opaque;
+
+    kbd = qobject_to_qdict(data);
+    monitor_printf(mon, "%c Keyboard #%" PRId64 ": %s\n",
+                  (qdict_get_bool(kbd, "current") ? '*' : ' '),
+                  qdict_get_int(kbd, "index"), qdict_get_str(kbd, "name"));
+}
+
+void do_info_keyboard_print(Monitor *mon, const QObject *data)
+{
+    QList *kbd_list;
+
+    kbd_list = qobject_to_qlist(data);
+    if (qlist_empty(kbd_list)) {
+        monitor_printf(mon, "No keyboard devices connected\n");
+        return;
+    }
+
+    qlist_iter(kbd_list, info_keyboard_iter, mon);
+}
+
+/*
+ * do_info_keyboard(): Show VM keyboard information
+ *
+ * Each keyboard is represented by a QDict, the returned QObject is
+ * a QList of all keyboards.
+ *
+ * The keyboard QDict contains the following:
+ *
+ * - "name": keyboard's name
+ * - "index": keyboard's index
+ * - "current": true if this keyboard is receiving events, false otherwise
+ *
+ * Example:
+ *
+ * [ { "name": "QEMU USB Keyboard", "index": 0, "current": false },
+ *   { "name": "QEMU PS/2 Keyboard", "index": 1, "current": true } ]
+ */
+void do_info_keyboard(Monitor *mon, QObject **ret_data)
+{
+    QEMUPutKbdEntry *cursor;
+    QList *kbd_list;
+    int index = 0;
+
+    kbd_list = qlist_new();
+
+    if (!qemu_put_kbd_event_head) {
+        goto out;
+    }
+
+    cursor = qemu_put_kbd_event_head;
+    while (cursor != NULL) {
+        QObject *obj;

input.c +27 -0

+
+/*
+ * do_keyboard_set(): Set active keyboard
+ *
+ * Argument qdict contains
+ * - "index": the keyboard index to set
+ *
+ * Example:
+ *
+ * { "index": "0" }
+ */
+int do_keyboard_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    QEMUPutKbdEntry *cursor;
+    int i = 0;
+    int index = qdict_get_int(qdict, "index");
+
+    if (!qemu_put_kbd_event_head) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, "keyboard");
+        return -1;
+    }
+
+    cursor = qemu_put_kbd_event_head;
+    while (cursor != NULL && index != i) {
+        i++;
+        cursor = cursor->next;
+    }

monitor.c +8 -0

index 0448a70..cc95b3d 100644
@@ -2783,6 +2783,14 @@ static const mon_cmd_t info_cmds[] = {
         .mhandler.info_new = do_info_mice,
     },
     {
+        .name       = "keyboard",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show which guest keyboard is receiving events",
+        .user_print = do_info_keyboard_print,
+        .mhandler.info_new = do_info_keyboard,
+    },
+    {
         .name       = "vnc",
         .args_type  = "",
         .params     = "",

qemu-monitor.hx +17 -0

index 5308f36..e9beb12 100644
@@ -659,6 +659,23 @@ info mice
 @end example
 ETEXI
 
+    {
+        .name       = "keyboard_set",
+        .args_type  = "index:i",
+        .params     = "index",
+        .help       = "set which keyboard device receives events",
+        .mhandler.cmd_new = do_keyboard_set,
+    },
+
+STEXI
+@item keyboard_set @var{index}
+@findex keyboard_set
+Set which keyboard device receives events at given @var{index}, index
+can be obtained with
+@example
+info keyboard
+@end example
+ETEXI
 #ifdef HAS_AUDIO
     {
         .name       = "wavcapture",

qerror.c +0 -0

index d0aba61..42a5cf7 100644
@@ -172,6 +172,14 @@ static const QErrorStringTable qerror_table[] = {
         .error_fmt = QERR_VNC_SERVER_FAILED,
         .desc      = "Could not start VNC server on %(target)",
     },

Date: Fri, 26 Mar 2010 10:57:58 +0100
From: Markus Armbruster <[email protected]>
To: Shahar Havivi <[email protected]>
Cc: [email protected]
Subject: Re: [Qemu-devel] [PATCH 2/2] Added monitor commands:
'keyboard_set' and 'info keybaord'

Quoted 95 lines · Shahar Havivi

Two new monitor commands: adding ability to handle which keyboard qemu will use and to see which keyboard are currently available.

$ info keyboard
$ keyboard_set <index>

Signed-off-by Shahar Havivi <[email protected]>

---
 console.h       |    4 ++
 input.c         |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 monitor.c       |    8 ++++
 qemu-monitor.hx |   17 +++++++++
 qerror.c        |    8 ++++
 5 files changed, 141 insertions(+), 0 deletions(-)

console.h +4 -0

index 16c9c3d..7efa88e 100644
@@ -85,6 +85,10 @@ void do_info_mice_print(Monitor *mon, const QObject *data);
 void do_info_mice(Monitor *mon, QObject **ret_data);
 void do_mouse_set(Monitor *mon, const QDict *qdict);
 
+void do_info_keyboard_print(Monitor *mon, const QObject *data);
+void do_info_keyboard(Monitor *mon, QObject **ret_data);
+int do_keyboard_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
+
 /* keysym is a unicode code except for special keys (see QEMU_KEY_xxx
    constants) */
 #define QEMU_KEY_ESC1(c) ((c) | 0xe100)

input.c +57 -0

index 563ecad..4daaeb0 100644
@@ -368,3 +368,107 @@ void qemu_remove_mouse_mode_change_notifier(Notifier *notify)
 {
     notifier_list_remove(&mouse_mode_notifiers, notify);
 }
+
+static void info_keyboard_iter(QObject *data, void *opaque)
+{
+    QDict *kbd;
+    Monitor *mon = opaque;
+
+    kbd = qobject_to_qdict(data);
+    monitor_printf(mon, "%c Keyboard #%" PRId64 ": %s\n",
+                  (qdict_get_bool(kbd, "current") ? '*' : ' '),
+                  qdict_get_int(kbd, "index"), qdict_get_str(kbd, "name"));
+}
+
+void do_info_keyboard_print(Monitor *mon, const QObject *data)
+{
+    QList *kbd_list;
+
+    kbd_list = qobject_to_qlist(data);
+    if (qlist_empty(kbd_list)) {
+        monitor_printf(mon, "No keyboard devices connected\n");
+        return;
+    }
+
+    qlist_iter(kbd_list, info_keyboard_iter, mon);
+}
+
+/*
+ * do_info_keyboard(): Show VM keyboard information
+ *
+ * Each keyboard is represented by a QDict, the returned QObject is
+ * a QList of all keyboards.
+ *
+ * The keyboard QDict contains the following:
+ *
+ * - "name": keyboard's name
+ * - "index": keyboard's index
+ * - "current": true if this keyboard is receiving events, false otherwise
+ *
+ * Example:
+ *
+ * [ { "name": "QEMU USB Keyboard", "index": 0, "current": false },
+ *   { "name": "QEMU PS/2 Keyboard", "index": 1, "current": true } ]
+ */
+void do_info_keyboard(Monitor *mon, QObject **ret_data)
+{
+    QEMUPutKbdEntry *cursor;
+    QList *kbd_list;
+    int index = 0;
+
+    kbd_list = qlist_new();
+
+    if (!qemu_put_kbd_event_head) {
+        goto out;
+    }
+
+    cursor = qemu_put_kbd_event_head;
+    while (cursor != NULL) {
+        QObject *obj;

input.c void qemu_remove_mouse_mode_change_notifier(Notifier *notify) · line 437

Quoted 10 lines

input.c +10 -0

+        obj = qobject_from_jsonf("{ 'name': %s, 'index': %d, 'current': %i }",
+                                 cursor->qemu_put_kbd_name,
+                                 index, cursor == qemu_put_kbd_event_current);
+        qlist_append_obj(kbd_list, obj);
+        index++;
+        cursor = cursor->next;
+    }
+out:
+    *ret_data = QOBJECT(kbd_list);
+}

Keyboard indexes change when keyboards other than the last one get removed. Hmm.

Quoted 27 lines

input.c +27 -0

+
+/*
+ * do_keyboard_set(): Set active keyboard
+ *
+ * Argument qdict contains
+ * - "index": the keyboard index to set
+ *
+ * Example:
+ *
+ * { "index": "0" }
+ */
+int do_keyboard_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    QEMUPutKbdEntry *cursor;
+    int i = 0;
+    int index = qdict_get_int(qdict, "index");
+
+    if (!qemu_put_kbd_event_head) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, "keyboard");
+        return -1;
+    }
+
+    cursor = qemu_put_kbd_event_head;
+    while (cursor != NULL && index != i) {
+        i++;
+        cursor = cursor->next;
+    }

input.c void qemu_remove_mouse_mode_change_notifier(Notifier *notify) · line 474

Quoted 10 lines

input.c +10 -0

+
+    if (cursor != NULL) {
+        qemu_put_kbd_event_current = cursor;
+    }
+    else {
+        qerror_report(QERR_INVALID_PARAMETER, "index");
+        return -1;
+    }
+    return 0;
+}

Since the index is not a stable identifier of keyboards, and we support multiple monitors, you're prone to remove the wrong keyboard.

Monitor#1 Monitor#2
"info keyboard" to find the index
unplug a keyboard, invalidating index
keyboard_set <index>

I think it's best to use a stable ID, like we do in other places. We commonly let the user specify it, e.g. as id=ID in -netdev, -device and elswhere.

Quoted 60 lines

monitor.c +8 -0

index 0448a70..cc95b3d 100644
@@ -2783,6 +2783,14 @@ static const mon_cmd_t info_cmds[] = {
         .mhandler.info_new = do_info_mice,
     },
     {
+        .name       = "keyboard",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show which guest keyboard is receiving events",
+        .user_print = do_info_keyboard_print,
+        .mhandler.info_new = do_info_keyboard,
+    },
+    {
         .name       = "vnc",
         .args_type  = "",
         .params     = "",

qemu-monitor.hx +17 -0

index 5308f36..e9beb12 100644
@@ -659,6 +659,23 @@ info mice
 @end example
 ETEXI
 
+    {
+        .name       = "keyboard_set",
+        .args_type  = "index:i",
+        .params     = "index",
+        .help       = "set which keyboard device receives events",
+        .mhandler.cmd_new = do_keyboard_set,
+    },
+
+STEXI
+@item keyboard_set @var{index}
+@findex keyboard_set
+Set which keyboard device receives events at given @var{index}, index
+can be obtained with
+@example
+info keyboard
+@end example
+ETEXI
 #ifdef HAS_AUDIO
     {
         .name       = "wavcapture",

qerror.c +4 -0

index d0aba61..42a5cf7 100644
@@ -172,6 +172,14 @@ static const QErrorStringTable qerror_table[] = {
         .error_fmt = QERR_VNC_SERVER_FAILED,
         .desc      = "Could not start VNC server on %(target)",
     },
+    {
+        .error_fmt = QERR_DEVICE_NOT_FOUND,
+        .desc      = "No keyboard device found",
+    },

qerror.c static const QErrorStringTable qerror_table[] = { · line 184

Quoted 6 lines

qerror.c +4 -0

+    {
+        .error_fmt = QERR_INVALID_PARAMETER,
+        .desc      = "Invalid index '%(name)' for keyboard device",
+    },
     {}
 };

I'm afraid you're breaking all other uses of these errors here.

On Fri, Mar 26, 2010 at 10:57:58AM +0100, Markus Armbruster wrote:

Quoted 9 lines

Since the index is not a stable identifier of keyboards, and we support multiple monitors, you're prone to remove the wrong keyboard.

Monitor#1 Monitor#2
"info keyboard" to find the index
unplug a keyboard, invalidating index
keyboard_set <index>

Quoted 3 lines

I think it's best to use a stable ID, like we do in other places. We commonly let the user specify it, e.g. as id=ID in -netdev, -device and elswhere.

What do we do when user not specify id for device?

Date: Wed, 31 Mar 2010 17:10:22 +0200
From: Markus Armbruster <[email protected]>
To: Shahar Havivi <[email protected]>
Cc: [email protected]
Subject: Re: [Qemu-devel] [PATCH 2/2] Added monitor commands:
'keyboard_set' and 'info keybaord'

Quoted 1 line · Shahar Havivi

On Fri, Mar 26, 2010 at 10:57:58AM +0100, Markus Armbruster wrote:

[...]

Quoted 12 lines

Quoted 12 lines

Since the index is not a stable identifier of keyboards, and we support multiple monitors, you're prone to remove the wrong keyboard.

Monitor#1 Monitor#2
"info keyboard" to find the index
unplug a keyboard, invalidating index
keyboard_set <index>

I think it's best to use a stable ID, like we do in other places. We commonly let the user specify it, e.g. as id=ID in -netdev, -device and elswhere.

Quoted 1 line

What do we do when user not specify id for device?

Two obvious options: make one up (problem: clashes), or stipulate "if you want to keyboard_set this keyboard, you must specify an id for it".

[...]

Quoted 1 line · Shahar Havivi

On Fri, Mar 26, 2010 at 10:57:58AM +0100, Markus Armbruster wrote:

[...]

Quoted 9 lines

Quoted 9 lines

Since the index is not a stable identifier of keyboards, and we support multiple monitors, you're prone to remove the wrong keyboard.

Monitor#1 Monitor#2
"info keyboard" to find the index
unplug a keyboard, invalidating index
keyboard_set <index>

Quoted 4 lines

Quoted 3 lines

I think it's best to use a stable ID, like we do in other places. We commonly let the user specify it, e.g. as id=ID in -netdev, -device and elswhere.

What do we do when user not specify id for device?

Two obvious options: make one up (problem: clashes), or stipulate "if you want to keyboard_set this keyboard, you must specify an id for it".