[Qemu-devel] [PATCH v2] un-register kbd driver for USB kbd unplug

[Qemu-devel] [PATCH v2] un-register kbd driver for USB kbd unplug

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

Kevin pointed out the TODO comment was obsolete and also suggested
turning the if lines into a switch statement. It seems cleaner, so
here is a second version of the patch.

This is a fairly simple fix for the problem where the keyboard event
handler is left in place when a keyboard is unplugged, eg. USB.

The long term solution would be to use Shahar Havivi's multi-keyboard
support patch[1] from March, but until Shahar's patch is ready, this one
fixes the current crash problem.

Cheers,
Jes

[1]: http://www.mail-archive.com/qemu-devel@nongnu.org/msg28589.html


Jes Sorensen (1):
  un-register kbd driver in case of USB kbd unplug.

 console.h    |    1 +
 hw/usb-hid.c |    8 ++++++--
 input.c      |    6 ++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

[Qemu-devel] [PATCH] un-register kbd driver in case of USB kbd unplug.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

If a USB keyboard is unplugged, the keyboard eventhandler is never
removed, and events will continue to be passed through to the device,
causing crashes or memory corruption.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 console.h    |    1 +
 hw/usb-hid.c |    8 ++++++--
 input.c      |    6 ++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/console.h b/console.h
index cac959f..aafb031 100644
--- a/console.h
+++ b/console.h
@@ -42,6 +42,7 @@ typedef struct QEMUPutLEDEntry {
 } QEMUPutLEDEntry;
 
 void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
+void qemu_remove_kbd_event_handler(void);
 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 8e6c6e0..ca04511 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -854,9 +854,13 @@ static void usb_hid_handle_destroy(USBDevice *dev)
 {
     USBHIDState *s = (USBHIDState *)dev;
 
-    if (s->kind != USB_KEYBOARD)
+    switch(s->kind) {
+    case USB_KEYBOARD:
+        qemu_remove_kbd_event_handler();
+        break;
+    default:
         qemu_remove_mouse_event_handler(s->ptr.eh_entry);
-    /* TODO: else */
+    }
 }
 
 static int usb_hid_initfn(USBDevice *dev, int kind)
diff --git a/input.c b/input.c
index 651442d..ec05548 100644
--- a/input.c
+++ b/input.c
@@ -42,6 +42,12 @@ void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
     qemu_put_kbd_event = func;
 }
 
+void qemu_remove_kbd_event_handler(void)
+{
+    qemu_put_kbd_event_opaque = NULL;
+    qemu_put_kbd_event = NULL;
+}
+
 static void check_mode_change(void)
 {
     static int current_is_absolute, current_has_absolute;
-- 
1.6.5.2

Re: [Qemu-devel] [PATCH] un-register kbd driver in case of USB kbd unplug.

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> If a USB keyboard is unplugged, the keyboard eventhandler is never
> removed, and events will continue to be passed through to the device,
> causing crashes or memory corruption.

Looks good.

I wonder how many more unplug surprises to expect.

Re: [Qemu-devel] [PATCH] un-register kbd driver in case of USB kbd unplug.

[Anthony Liguori]

On 06/08/2010 08:12 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> If a USB keyboard is unplugged, the keyboard eventhandler is never
> removed, and events will continue to be passed through to the device,
> causing crashes or memory corruption.
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   console.h    |    1 +
>   hw/usb-hid.c |    8 ++++++--
>   input.c      |    6 ++++++
>   3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/console.h b/console.h
> index cac959f..aafb031 100644
> --- a/console.h
> +++ b/console.h
> @@ -42,6 +42,7 @@ typedef struct QEMUPutLEDEntry {
>   } QEMUPutLEDEntry;
>
>   void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
> +void qemu_remove_kbd_event_handler(void);
>   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 8e6c6e0..ca04511 100644
> --- a/hw/usb-hid.c
> +++ b/hw/usb-hid.c
> @@ -854,9 +854,13 @@ static void usb_hid_handle_destroy(USBDevice *dev)
>   {
>       USBHIDState *s = (USBHIDState *)dev;
>
> -    if (s->kind != USB_KEYBOARD)
> +    switch(s->kind) {
> +    case USB_KEYBOARD:
> +        qemu_remove_kbd_event_handler();
> +        break;
> +    default:
>           qemu_remove_mouse_event_handler(s->ptr.eh_entry);
> -    /* TODO: else */
> +    }
>   }
>
>   static int usb_hid_initfn(USBDevice *dev, int kind)
> diff --git a/input.c b/input.c
> index 651442d..ec05548 100644
> --- a/input.c
> +++ b/input.c
> @@ -42,6 +42,12 @@ void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
>       qemu_put_kbd_event = func;
>   }
>
> +void qemu_remove_kbd_event_handler(void)
> +{
> +    qemu_put_kbd_event_opaque = NULL;
> +    qemu_put_kbd_event = NULL;
> +}
> +
>   static void check_mode_change(void)
>   {
>       static int current_is_absolute, current_has_absolute;
>