Re: [Qemu-devel] [PATCH 1/2] Support for multiple keyboard devices

[Markus Armbruster]

Shahar Havivi <address@hidden> writes:

> 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 <address@hidden>
> ---
>  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(-)
>
> diff --git a/console.h b/console.h
> index 6def115..16c9c3d 100644
> --- a/console.h
> +++ b/console.h
> @@ -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);
[...]
> diff --git a/input.c b/input.c
> index 8f0941e..563ecad 100644
> --- a/input.c
> +++ b/input.c
> @@ -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;
> +
> +    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;
> +    }

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

> +
> +    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);
> +        return;
> +    }
> +
> +    prev->next = entry->next;
> +
> +    if (qemu_put_kbd_event_current == entry) {
> +        qemu_put_kbd_event_current = prev;
> +    }
> +
> +    qemu_free(entry);

More list pointer chasing.

> +}
> +
>  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);
>      }
>  }