From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NrxTH-00086f-BV for qemu-devel@nongnu.org; Wed, 17 Mar 2010 13:55:55 -0400 Received: from [199.232.76.173] (port=55461 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NrxTG-00086N-TB for qemu-devel@nongnu.org; Wed, 17 Mar 2010 13:55:54 -0400 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NrxTE-0007hy-Uh for qemu-devel@nongnu.org; Wed, 17 Mar 2010 13:55:54 -0400 Received: from mail-pz0-f194.google.com ([209.85.222.194]:48412) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NrxTE-0007hs-GF for qemu-devel@nongnu.org; Wed, 17 Mar 2010 13:55:52 -0400 Received: by pzk32 with SMTP id 32so919930pzk.4 for ; Wed, 17 Mar 2010 10:55:51 -0700 (PDT) Message-ID: <4BA117A2.5020309@codemonkey.ws> Date: Wed, 17 Mar 2010 12:55:46 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Support for multiple keyboard device References: <20100311215712.GA6606@redhat.com> In-Reply-To: <20100311215712.GA6606@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shahar Havivi Cc: Dor Laor , qemu-devel@nongnu.org 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 > 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