Re: [Qemu-devel] [PATCH] Support for multiple keyboard device

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/11/2010 03:57 PM, Shahar Havivi wrote:
> Currently qemu use the last keyboard device that added,
> When removing keyboard (via device_del kbd) you get segfault next time
> you try to write in the client.
>
> i.e. start qemu
>    x86_64-softmmu/qemu-system-x86_64 -usb -device usb-kbd,id=kbd
> switch to monitor
>    device_del kbd
> switch back to client, segfault
>
> This patch fix the segfault and add list of all the keyboard handle much
> like the mouse device does.
>
>
> Signed-off-by: Shahar Havivi<shaharh@redhat.com>
>    

It's a good idea, but I'd like to commit my rework of input.c first 
(which I'll do in a couple hours).  A few comments:

> ---
>   console.h    |    9 +++++-
>   hw/usb-hid.c |    9 ++++--
>   input.c      |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>   3 files changed, 88 insertions(+), 11 deletions(-)
>
> diff --git a/console.h b/console.h
> index 71e8ff2..e250008 100644
> --- a/console.h
> +++ b/console.h
> @@ -38,7 +38,14 @@ typedef struct QEMUPutLEDEntry {
>       QTAILQ_ENTRY(QEMUPutLEDEntry) next;
>   } QEMUPutLEDEntry;
>
> -void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
> +typedef struct QEMUPutKbdEntry {
> +    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);
> +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/hw/usb-hid.c b/hw/usb-hid.c
> index 2e4e647..1dd0cc9 100644
> --- a/hw/usb-hid.c
> +++ b/hw/usb-hid.c
> @@ -55,6 +55,7 @@ typedef struct USBKeyboardState {
>       uint8_t leds;
>       uint8_t key[16];
>       int keys;
> +    QEMUPutKbdEntry *eh_entry;
>   } USBKeyboardState;
>
>   typedef struct USBHIDState {
> @@ -635,7 +636,7 @@ static void usb_keyboard_handle_reset(USBDevice *dev)
>   {
>       USBHIDState *s = (USBHIDState *)dev;
>
> -    qemu_add_kbd_event_handler(usb_keyboard_event, s);
> +    s->kbd.eh_entry = qemu_add_kbd_event_handler(usb_keyboard_event, s);
>       s->protocol = 1;
>   }
>
> @@ -856,9 +857,11 @@ static void usb_hid_handle_destroy(USBDevice *dev)
>   {
>       USBHIDState *s = (USBHIDState *)dev;
>
> -    if (s->kind != USB_KEYBOARD)
> +    if (s->kind != USB_KEYBOARD) {
>           qemu_remove_mouse_event_handler(s->ptr.eh_entry);
> -    /* TODO: else */
> +    } else {
> +        qemu_remove_kbd_event_handler(s->kbd.eh_entry);
> +    }
>   }
>
>   static int usb_hid_initfn(USBDevice *dev, int kind)
> diff --git a/input.c b/input.c
> index baaa4c6..90b6cfb 100644
> --- a/input.c
> +++ b/input.c
> @@ -29,16 +29,82 @@
>   #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 QEMUPutMouseEntry *qemu_put_mouse_event_head;
>   static QEMUPutMouseEntry *qemu_put_mouse_event_current;
>   static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers);
>
> -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 *s, *cursor;
> +
> +    cursor = qemu_put_kbd_event_head;
> +    while (cursor) {
> +        if (cursor->qemu_put_kbd_event == func&&
> +            cursor->qemu_put_kbd_event_opaque == opaque) {
> +
> +            qemu_put_kbd_event_current = cursor;
> +            return cursor;
> +        }
> +        cursor = cursor->next;
> +    }
> +
> +    s = qemu_mallocz(sizeof(QEMUPutKbdEntry));
> +
> +    s->qemu_put_kbd_event_opaque = opaque;
> +    s->qemu_put_kbd_event = func;
> +    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);
> +        return;
> +    }
> +
> +    prev->next = entry->next;
> +
> +    if (qemu_put_kbd_event_current == entry) {
> +        qemu_put_kbd_event_current = prev;
> +    }
> +
> +    qemu_free(entry);
>   }
>
>   QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
> @@ -126,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);
>       }
>   }
>
>    

Shouldn't we have a keyboard_set monitor command (much like the 
mouse_set command)?  Also, it would be helpful to have an info keyboard.

Regards,

Anthony Liguori