[Qemu-devel] [PATCH v2 0/2] input: add keyboard handler list

[Qemu-devel] [PATCH v2 0/2] input: add keyboard handler list

[Gerd Hoffmann]

  Hi,

Little input patch series, first patch being a small cleanup, second
patch adds a linked list of keyboard handlers, so removing a handler
(on usb kbd unplug) will not make keyboard stop working but fallback
to another keyboard (if present).

cheers,
  Gerd

Gerd Hoffmann (2):
  input: make QEMUPutLEDEntry + QEMUPutMouseEntry private
  input: introduce keyboard handler list

 hw/hid.c             |    4 ++--
 hw/hid.h             |    1 +
 include/ui/console.h |   25 ++++++-----------------
 ui/input.c           |   55 +++++++++++++++++++++++++++++++++++++++-----------
 4 files changed, 52 insertions(+), 33 deletions(-)

-- 
1.7.9.7

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

[Gerd Hoffmann]

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.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/console.h |   19 ++-----------------
 ui/input.c           |   18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index c42bca6..ce5a550 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -27,23 +27,8 @@ typedef void QEMUPutKBDEvent(void *opaque, int keycode);
 typedef void QEMUPutLEDEvent(void *opaque, int ledstate);
 typedef void QEMUPutMouseEvent(void *opaque, int dx, int dy, int dz, int buttons_state);
 
-typedef struct QEMUPutMouseEntry {
-    QEMUPutMouseEvent *qemu_put_mouse_event;
-    void *qemu_put_mouse_event_opaque;
-    int qemu_put_mouse_event_absolute;
-    char *qemu_put_mouse_event_name;
-
-    int index;
-
-    /* used internally by qemu for handling mice */
-    QTAILQ_ENTRY(QEMUPutMouseEntry) node;
-} QEMUPutMouseEntry;
-
-typedef struct QEMUPutLEDEntry {
-    QEMUPutLEDEvent *put_led;
-    void *opaque;
-    QTAILQ_ENTRY(QEMUPutLEDEntry) next;
-} QEMUPutLEDEntry;
+typedef struct QEMUPutMouseEntry QEMUPutMouseEntry;
+typedef struct QEMUPutLEDEntry QEMUPutLEDEntry;
 
 void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
 void qemu_remove_kbd_event_handler(void);
diff --git a/ui/input.c b/ui/input.c
index 9abef0c..87a23df 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -29,6 +29,24 @@
 #include "qmp-commands.h"
 #include "qapi-types.h"
 
+struct QEMUPutMouseEntry {
+    QEMUPutMouseEvent *qemu_put_mouse_event;
+    void *qemu_put_mouse_event_opaque;
+    int qemu_put_mouse_event_absolute;
+    char *qemu_put_mouse_event_name;
+
+    int index;
+
+    /* used internally by qemu for handling mice */
+    QTAILQ_ENTRY(QEMUPutMouseEntry) node;
+};
+
+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);
-- 
1.7.9.7

[Qemu-devel] [PATCH v2 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/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..57e963b 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[] = {
@@ -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);
     }
 }
 
-- 
1.7.9.7

Re: [Qemu-devel] [PATCH v2 0/2] input: add keyboard handler list

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
> Little input patch series, first patch being a small cleanup, second
> patch adds a linked list of keyboard handlers, so removing a handler
> (on usb kbd unplug) will not make keyboard stop working but fallback
> to another keyboard (if present).

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

Re: [Qemu-devel] [PATCH v2 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.
>
> [ v2: fix cut+paste bug found my Markus ]

In the future, please put patch history information below the --- line.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

Put it here.  Thanks!

>  hw/hid.c             |    4 ++--
>  hw/hid.h             |    1 +
>  include/ui/console.h |    6 ++++--
>  ui/input.c           |   37 +++++++++++++++++++++++++------------
>  4 files changed, 32 insertions(+), 16 deletions(-)
[...]

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

[Gerd Hoffmann]

On 03/11/13 10:25, Markus Armbruster wrote:
> 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.
>>
>> [ v2: fix cut+paste bug found my Markus ]
> 
> In the future, please put patch history information below the --- line.
> 
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
> 
> Put it here.  Thanks!

How do I get it there?

thanks,
  Gerd

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

[Anthony Liguori]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On 03/11/13 10:25, Markus Armbruster wrote:
>> 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.
>>>
>>> [ v2: fix cut+paste bug found my Markus ]
>> 
>> In the future, please put patch history information below the --- line.
>> 
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>> 
>> Put it here.  Thanks!
>
> How do I get it there?

You can just stick a '---' prior to the history in the git commit.
git-format-patch and git-am will then do the right thing.

Just make sure you don't push those commits.  Process them via git-am
like you would a patch from someone else.

Regards,

Anthony Liguori

>
> thanks,
>   Gerd

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

[Peter Maydell]

On 8 March 2013 14:22, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 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.

Ah, multiple keyboard handlers. I've actually been carrying
something similar in the qemu-linaro tree for ages, inherited
from Nokia for n900 support:
http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=commitdiff;h=6be1ca3d2b79118fce871aa8454336502b3a5ca4

The major difference is that where this code sends key events
to the first handler only, the Nokia patch sends events to
all keys, because the use case it's trying to deal with is
"some keys should go to one device and some to another"
(for example, most keys go to a usb keyboard but F-keys
are treated as being like some of the hardware buttons on
the phone, and they go to a different device in the model).

Maybe we could deal with this by having kbd_put_keycode()
send the event to every register keyboard until one of
them says it has handled it [ie have the put_kbd callback
return a handled/not-handled return]. That would still
leave the issue that you need to make sure that keyboard
handlers get called in the right order (perhaps a priority
thing on registering?)

Anyway, no objection to this patch (and I'm certainly not
trying to say you should fix my problems in this patch),
I just thought I'd mention this other usecase for multiple
keyboards while it was in my head.

thanks
-- PMM

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

[Markus Armbruster]

Anthony Liguori <aliguori@us.ibm.com> writes:

> Gerd Hoffmann <kraxel@redhat.com> writes:
>
>> On 03/11/13 10:25, Markus Armbruster wrote:
>>> 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.
>>>>
>>>> [ v2: fix cut+paste bug found my Markus ]
>>> 
>>> In the future, please put patch history information below the --- line.
>>> 
>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>> ---
>>> 
>>> Put it here.  Thanks!
>>
>> How do I get it there?
>
> You can just stick a '---' prior to the history in the git commit.
> git-format-patch and git-am will then do the right thing.
>
> Just make sure you don't push those commits.  Process them via git-am
> like you would a patch from someone else.

Another convenient place is the cover letter.

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

[Gerd Hoffmann]

  Hi,

> The major difference is that where this code sends key events
> to the first handler only, the Nokia patch sends events to
> all keys, because the use case it's trying to deal with is
> "some keys should go to one device and some to another"
> (for example, most keys go to a usb keyboard but F-keys
> are treated as being like some of the hardware buttons on
> the phone, and they go to a different device in the model).

> Maybe we could deal with this by having kbd_put_keycode()
> send the event to every register keyboard until one of
> them says it has handled it [ie have the put_kbd callback
> return a handled/not-handled return].

Yea, sending to all doesn't fly in general as guests
with ps/2 + usb kbd would see all keys twice then ...

Having a "handled" return code makes sense to me.  IIRC glib has
something simliar (return true == handled, return false == call next
handler please).

> That would still
> leave the issue that you need to make sure that keyboard
> handlers get called in the right order (perhaps a priority
> thing on registering?)

A priority field is more robust I think.

> Anyway, no objection to this patch (and I'm certainly not
> trying to say you should fix my problems in this patch),
> I just thought I'd mention this other usecase for multiple
> keyboards while it was in my head.

Should be easy to add support for this on top of this series.

cheers,
  Gerd

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

[Anthony Liguori]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> The major difference is that where this code sends key events
>> to the first handler only, the Nokia patch sends events to
>> all keys, because the use case it's trying to deal with is
>> "some keys should go to one device and some to another"
>> (for example, most keys go to a usb keyboard but F-keys
>> are treated as being like some of the hardware buttons on
>> the phone, and they go to a different device in the model).
>
>> Maybe we could deal with this by having kbd_put_keycode()
>> send the event to every register keyboard until one of
>> them says it has handled it [ie have the put_kbd callback
>> return a handled/not-handled return].
>
> Yea, sending to all doesn't fly in general as guests
> with ps/2 + usb kbd would see all keys twice then ...
>
> Having a "handled" return code makes sense to me.  IIRC glib has
> something simliar (return true == handled, return false == call next
> handler please).
>
>> That would still
>> leave the issue that you need to make sure that keyboard
>> handlers get called in the right order (perhaps a priority
>> thing on registering?)
>
> A priority field is more robust I think.
>
>> Anyway, no objection to this patch (and I'm certainly not
>> trying to say you should fix my problems in this patch),
>> I just thought I'd mention this other usecase for multiple
>> keyboards while it was in my head.
>
> Should be easy to add support for this on top of this series.

I don't really have an issue with this series.  It's a reasonable
incremental step.  But I wrote an ps2 qtest recently and had some
thoughts about how we handle input and how we could do it in a better
fashion.

We don't really have the notion of an input device backend/frontend.
There's no great way for a user to see what input devices there are or
participate in which ones should have input.  QOM can fill this void.

We could make a QOM interface, InputInterface, loosely modelled
after the Linux input layer.

The interface would have methods like:

struct InputInterface {
    uint32_t (*get_capabilities)(InputInterface *iface);
    const char *(get_description)(InputInterface *iface);

    void (*begin_batch)(InputInterface *iface);
    void (*event)(InputInterface *iface, uint16_t type,
                  uint16_t code, int32_t value);
    void (*end_batch)(InputInterface *iface);
};

Then any device capable of acting as an input device would implement
this interface.  qom-list can then be used to enumerate any Input
devices.  The VNC server (or GTK UI) could also be modelled as an object
and you could select which input device received keyboard input by
hooking up the properties.

It also makes the devices easier to unit test because you can call the
interface methods directly.

An interface like this would also work for richer input mechanisms like
touchscreens.

Regards,

Anthony Liguori


>
> cheers,
>   Gerd