Re: [Qemu-devel] [PATCH 2/2] input: introduce keyboard handler list

Re: [Qemu-devel] [PATCH 2/2] input: introduce keyboard handler list

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Add a linked list of keyboard handlers.  Added handlers will go
> to the head of the list.  Removed handlers will be zapped from
> the list.  The head of the list will be used for events.
>
> This fixes the keyboard-dead-after-usb-kbd-unplug issue, key events
> will be re-routed to the ps/2 kbd instead of being discarded.

Patterned after the mouse code, which already has such a list.

For mice, we have a monitor command mouse_set to select the active one.
Useful for keyboards, too?  I dimly recall somebody trying something
like that in the past.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/hid.c             |    4 ++--
>  hw/hid.h             |    1 +
>  include/ui/console.h |    6 ++++--
>  ui/input.c           |   37 +++++++++++++++++++++++++------------
>  4 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/hw/hid.c b/hw/hid.c
> index 89b5415..6be00ec 100644
> --- a/hw/hid.c
> +++ b/hw/hid.c
> @@ -415,7 +415,7 @@ void hid_free(HIDState *hs)
>  {
>      switch (hs->kind) {
>      case HID_KEYBOARD:
> -        qemu_remove_kbd_event_handler();
> +        qemu_remove_kbd_event_handler(hs->kbd.eh_entry);
>          break;
>      case HID_MOUSE:
>      case HID_TABLET:
> @@ -431,7 +431,7 @@ void hid_init(HIDState *hs, int kind, HIDEventFunc event)
>      hs->event = event;
>  
>      if (hs->kind == HID_KEYBOARD) {
> -        qemu_add_kbd_event_handler(hid_keyboard_event, hs);
> +        hs->kbd.eh_entry = qemu_add_kbd_event_handler(hid_keyboard_event, hs);
>      } else if (hs->kind == HID_MOUSE) {
>          hs->ptr.eh_entry = qemu_add_mouse_event_handler(hid_pointer_event, hs,
>                                                          0, "QEMU HID Mouse");
> diff --git a/hw/hid.h b/hw/hid.h
> index 56c71ed..2567879 100644
> --- a/hw/hid.h
> +++ b/hw/hid.h
> @@ -31,6 +31,7 @@ typedef struct HIDKeyboardState {
>      uint8_t leds;
>      uint8_t key[16];
>      int32_t keys;
> +    QEMUPutKbdEntry *eh_entry;
>  } HIDKeyboardState;
>  
>  struct HIDState {
> diff --git a/include/ui/console.h b/include/ui/console.h
> index ce5a550..fd39d94 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -28,10 +28,12 @@ typedef void QEMUPutLEDEvent(void *opaque, int ledstate);
>  typedef void QEMUPutMouseEvent(void *opaque, int dx, int dy, int dz, int buttons_state);
>  
>  typedef struct QEMUPutMouseEntry QEMUPutMouseEntry;
> +typedef struct QEMUPutKbdEntry QEMUPutKbdEntry;
>  typedef struct QEMUPutLEDEntry QEMUPutLEDEntry;
>  
> -void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
> -void qemu_remove_kbd_event_handler(void);
> +QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func,
> +                                            void *opaque);
> +void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry);
>  QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
>                                                  void *opaque, int absolute,
>                                                  const char *name);
> diff --git a/ui/input.c b/ui/input.c
> index 87a23df..59d0578 100644
> --- a/ui/input.c
> +++ b/ui/input.c
> @@ -41,18 +41,25 @@ struct QEMUPutMouseEntry {
>      QTAILQ_ENTRY(QEMUPutMouseEntry) node;
>  };
>  
> +struct QEMUPutKbdEntry {
> +    QEMUPutLEDEvent *put_kbd;

Sure it's not QEMUPutKbdEvent?

> +    void *opaque;
> +    QTAILQ_ENTRY(QEMUPutKbdEntry) next;
> +};
> +
>  struct QEMUPutLEDEntry {
>      QEMUPutLEDEvent *put_led;
>      void *opaque;
>      QTAILQ_ENTRY(QEMUPutLEDEntry) next;
>  };
>  
> -static QEMUPutKBDEvent *qemu_put_kbd_event;
> -static void *qemu_put_kbd_event_opaque;
> -static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers);
> +static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers =
> +    QTAILQ_HEAD_INITIALIZER(led_handlers);

Unrelated style cleanup.  Tolerable.

> +static QTAILQ_HEAD(, QEMUPutKbdEntry) kbd_handlers =
> +    QTAILQ_HEAD_INITIALIZER(kbd_handlers);
>  static QTAILQ_HEAD(, QEMUPutMouseEntry) mouse_handlers =
>      QTAILQ_HEAD_INITIALIZER(mouse_handlers);
> -static NotifierList mouse_mode_notifiers = 
> +static NotifierList mouse_mode_notifiers =
>      NOTIFIER_LIST_INITIALIZER(mouse_mode_notifiers);

Unrelated whitespace cleanup.  Tolerable.

>  
>  static const int key_defs[] = {
> @@ -306,16 +313,20 @@ void qmp_send_key(KeyValueList *keys, bool has_hold_time, int64_t hold_time,
>                     muldiv64(get_ticks_per_sec(), hold_time, 1000));
>  }
>  
> -void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
> +QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
>  {
> -    qemu_put_kbd_event_opaque = opaque;
> -    qemu_put_kbd_event = func;
> +    QEMUPutKbdEntry *entry;
> +
> +    entry = g_malloc0(sizeof(QEMUPutKbdEntry));
> +    entry->put_kbd = func;
> +    entry->opaque = opaque;
> +    QTAILQ_INSERT_HEAD(&kbd_handlers, entry, next);
> +    return entry;
>  }
>  
> -void qemu_remove_kbd_event_handler(void)
> +void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry)
>  {
> -    qemu_put_kbd_event_opaque = NULL;
> -    qemu_put_kbd_event = NULL;
> +    QTAILQ_REMOVE(&kbd_handlers, entry, next);
>  }
>  
>  static void check_mode_change(void)
> @@ -399,11 +410,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
>  
>  void kbd_put_keycode(int keycode)
>  {
> +    QEMUPutKbdEntry *entry = QTAILQ_FIRST(&kbd_handlers);
> +
>      if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) {
>          return;
>      }
> -    if (qemu_put_kbd_event) {
> -        qemu_put_kbd_event(qemu_put_kbd_event_opaque, keycode);
> +    if (entry) {
> +        entry->put_kbd(entry->opaque, keycode);
>      }
>  }

Re: [Qemu-devel] [PATCH 1/2] input: make QEMUPutLEDEntry + QEMUPutMouseEntry private

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> There is no need for anybody outside ui/input.c to access the
> struct elements.  Move the definitions, leaving only the typedefs
> in the header files.

No-brainer (assuming it compiles).

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] input: introduce keyboard handler list

[Gerd Hoffmann]

  Hi,

> For mice, we have a monitor command mouse_set to select the active one.
> Useful for keyboards, too?  I dimly recall somebody trying something
> like that in the past.

Don't feel like doing that now.

First, isn't important enough IMHO.

Second, I'm busy cleaning up console + display code.  With this done it
should be easy to have proper multihead support (not the somewhat
hackish way we do it with spice now, where the qemu console / display
code is completely bypassed by secondary qxl cards).  And once we have
that we'll probably wanna have input *routing* instead of input
*selection*, i.e. input from this ui window goes to that virtual
mouse/keyboard, so we can emulate multiseat setups ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] input: introduce keyboard handler list

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> For mice, we have a monitor command mouse_set to select the active one.
>> Useful for keyboards, too?  I dimly recall somebody trying something
>> like that in the past.
>
> Don't feel like doing that now.
>
> First, isn't important enough IMHO.
>
> Second, I'm busy cleaning up console + display code.  With this done it
> should be easy to have proper multihead support (not the somewhat
> hackish way we do it with spice now, where the qemu console / display
> code is completely bypassed by secondary qxl cards).  And once we have
> that we'll probably wanna have input *routing* instead of input
> *selection*, i.e. input from this ui window goes to that virtual
> mouse/keyboard, so we can emulate multiseat setups ...

Fair enough.  Fix the QEMUPutLEDEvent vs. QEMUPutKBDEvent pasto, and you
can have my reviwed-by.

[Qemu-devel] [PATCH 2/2] input: introduce keyboard handler list

[Gerd Hoffmann]

Add a linked list of keyboard handlers.  Added handlers will go
to the head of the list.  Removed handlers will be zapped from
the list.  The head of the list will be used for events.

This fixes the keyboard-dead-after-usb-kbd-unplug issue, key events
will be re-routed to the ps/2 kbd instead of being discarded.

[ v2: fix cut+paste bug found my Markus ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/input/hid.c         |    4 ++--
 include/hw/input/hid.h |    1 +
 include/ui/console.h   |    6 ++++--
 ui/input.c             |   37 +++++++++++++++++++++++++------------
 4 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/hw/input/hid.c b/hw/input/hid.c
index 5fbde98..14b3125 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -415,7 +415,7 @@ void hid_free(HIDState *hs)
 {
     switch (hs->kind) {
     case HID_KEYBOARD:
-        qemu_remove_kbd_event_handler();
+        qemu_remove_kbd_event_handler(hs->kbd.eh_entry);
         break;
     case HID_MOUSE:
     case HID_TABLET:
@@ -431,7 +431,7 @@ void hid_init(HIDState *hs, int kind, HIDEventFunc event)
     hs->event = event;
 
     if (hs->kind == HID_KEYBOARD) {
-        qemu_add_kbd_event_handler(hid_keyboard_event, hs);
+        hs->kbd.eh_entry = qemu_add_kbd_event_handler(hid_keyboard_event, hs);
     } else if (hs->kind == HID_MOUSE) {
         hs->ptr.eh_entry = qemu_add_mouse_event_handler(hid_pointer_event, hs,
                                                         0, "QEMU HID Mouse");
diff --git a/include/hw/input/hid.h b/include/hw/input/hid.h
index 56c71ed..2567879 100644
--- a/include/hw/input/hid.h
+++ b/include/hw/input/hid.h
@@ -31,6 +31,7 @@ typedef struct HIDKeyboardState {
     uint8_t leds;
     uint8_t key[16];
     int32_t keys;
+    QEMUPutKbdEntry *eh_entry;
 } HIDKeyboardState;
 
 struct HIDState {
diff --git a/include/ui/console.h b/include/ui/console.h
index 5cc5d0c..1c82f51 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -29,10 +29,12 @@ typedef void QEMUPutLEDEvent(void *opaque, int ledstate);
 typedef void QEMUPutMouseEvent(void *opaque, int dx, int dy, int dz, int buttons_state);
 
 typedef struct QEMUPutMouseEntry QEMUPutMouseEntry;
+typedef struct QEMUPutKbdEntry QEMUPutKbdEntry;
 typedef struct QEMUPutLEDEntry QEMUPutLEDEntry;
 
-void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
-void qemu_remove_kbd_event_handler(void);
+QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func,
+                                            void *opaque);
+void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry);
 QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name);
diff --git a/ui/input.c b/ui/input.c
index d8793e7..8ca1a03 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -41,18 +41,25 @@ struct QEMUPutMouseEntry {
     QTAILQ_ENTRY(QEMUPutMouseEntry) node;
 };
 
+struct QEMUPutKbdEntry {
+    QEMUPutKBDEvent *put_kbd;
+    void *opaque;
+    QTAILQ_ENTRY(QEMUPutKbdEntry) next;
+};
+
 struct QEMUPutLEDEntry {
     QEMUPutLEDEvent *put_led;
     void *opaque;
     QTAILQ_ENTRY(QEMUPutLEDEntry) next;
 };
 
-static QEMUPutKBDEvent *qemu_put_kbd_event;
-static void *qemu_put_kbd_event_opaque;
-static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers);
+static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers =
+    QTAILQ_HEAD_INITIALIZER(led_handlers);
+static QTAILQ_HEAD(, QEMUPutKbdEntry) kbd_handlers =
+    QTAILQ_HEAD_INITIALIZER(kbd_handlers);
 static QTAILQ_HEAD(, QEMUPutMouseEntry) mouse_handlers =
     QTAILQ_HEAD_INITIALIZER(mouse_handlers);
-static NotifierList mouse_mode_notifiers = 
+static NotifierList mouse_mode_notifiers =
     NOTIFIER_LIST_INITIALIZER(mouse_mode_notifiers);
 
 static const int key_defs[] = {
@@ -304,16 +311,20 @@ void qmp_send_key(KeyValueList *keys, bool has_hold_time, int64_t hold_time,
                    muldiv64(get_ticks_per_sec(), hold_time, 1000));
 }
 
-void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
+QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
 {
-    qemu_put_kbd_event_opaque = opaque;
-    qemu_put_kbd_event = func;
+    QEMUPutKbdEntry *entry;
+
+    entry = g_malloc0(sizeof(QEMUPutKbdEntry));
+    entry->put_kbd = func;
+    entry->opaque = opaque;
+    QTAILQ_INSERT_HEAD(&kbd_handlers, entry, next);
+    return entry;
 }
 
-void qemu_remove_kbd_event_handler(void)
+void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry)
 {
-    qemu_put_kbd_event_opaque = NULL;
-    qemu_put_kbd_event = NULL;
+    QTAILQ_REMOVE(&kbd_handlers, entry, next);
 }
 
 static void check_mode_change(void)
@@ -397,11 +408,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
 
 void kbd_put_keycode(int keycode)
 {
+    QEMUPutKbdEntry *entry = QTAILQ_FIRST(&kbd_handlers);
+
     if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) {
         return;
     }
-    if (qemu_put_kbd_event) {
-        qemu_put_kbd_event(qemu_put_kbd_event_opaque, keycode);
+    if (entry) {
+        entry->put_kbd(entry->opaque, keycode);
     }
 }
 
-- 
1.7.9.7