Patchwork [v2,1/4] kbd leds: infrastructure

login
register
mail settings
Submitter Gerd Hoffmann
Date Feb. 26, 2010, 4:17 p.m.
Message ID <1267201059-24145-2-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/46357/
State New
Headers show

Comments

Gerd Hoffmann - Feb. 26, 2010, 4:17 p.m.
Adds infrastructure for keyboard led status tracking to qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 console.h |   15 +++++++++++++++
 input.c   |   31 +++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 0 deletions(-)
Anthony Liguori - March 9, 2010, 3:02 p.m.
On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
> Adds infrastructure for keyboard led status tracking to qemu.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>    
Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   console.h |   15 +++++++++++++++
>   input.c   |   31 +++++++++++++++++++++++++++++++
>   2 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/console.h b/console.h
> index 916859d..71e8ff2 100644
> --- a/console.h
> +++ b/console.h
> @@ -10,10 +10,16 @@
>   #define MOUSE_EVENT_RBUTTON 0x02
>   #define MOUSE_EVENT_MBUTTON 0x04
>
> +/* identical to the ps/2 keyboard bits */
> +#define QEMU_SCROLL_LOCK_LED (1<<  0)
> +#define QEMU_NUM_LOCK_LED    (1<<  1)
> +#define QEMU_CAPS_LOCK_LED   (1<<  2)
> +
>   /* in ms */
>   #define GUI_REFRESH_INTERVAL 30
>
>   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 {
> @@ -26,13 +32,22 @@ typedef struct QEMUPutMouseEntry {
>       struct QEMUPutMouseEntry *next;
>   } QEMUPutMouseEntry;
>
> +typedef struct QEMUPutLEDEntry {
> +    QEMUPutLEDEvent *put_led;
> +    void *opaque;
> +    QTAILQ_ENTRY(QEMUPutLEDEntry) next;
> +} QEMUPutLEDEntry;
> +
>   void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
>   QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
>                                                   void *opaque, int absolute,
>                                                   const char *name);
>   void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry);
> +QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, void *opaque);
> +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
>
>   void kbd_put_keycode(int keycode);
> +void kbd_put_ledstate(int ledstate);
>   void kbd_mouse_event(int dx, int dy, int dz, int buttons_state);
>   int kbd_mouse_is_absolute(void);
>
> diff --git a/input.c b/input.c
> index 955b9ab..baaa4c6 100644
> --- a/input.c
> +++ b/input.c
> @@ -33,6 +33,7 @@ static QEMUPutKBDEvent *qemu_put_kbd_event;
>   static void *qemu_put_kbd_event_opaque;
>   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)
>   {
> @@ -102,6 +103,27 @@ void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry)
>       qemu_free(entry);
>   }
>
> +QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func,
> +                                            void *opaque)
> +{
> +    QEMUPutLEDEntry *s;
> +
> +    s = qemu_mallocz(sizeof(QEMUPutLEDEntry));
> +
> +    s->put_led = func;
> +    s->opaque = opaque;
> +    QTAILQ_INSERT_TAIL(&led_handlers, s, next);
> +    return s;
> +}
> +
> +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
> +{
> +    if (entry == NULL)
> +        return;
> +    QTAILQ_REMOVE(&led_handlers, entry, next);
> +    qemu_free(entry);
> +}
> +
>   void kbd_put_keycode(int keycode)
>   {
>       if (qemu_put_kbd_event) {
> @@ -109,6 +131,15 @@ void kbd_put_keycode(int keycode)
>       }
>   }
>
> +void kbd_put_ledstate(int ledstate)
> +{
> +    QEMUPutLEDEntry *cursor;
> +
> +    QTAILQ_FOREACH(cursor,&led_handlers, next) {
> +        cursor->put_led(cursor->opaque, ledstate);
> +    }
> +}
> +
>   void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
>   {
>       QEMUPutMouseEvent *mouse_event;
>
Paul Brook - March 9, 2010, 3:58 p.m.
> On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
> > Adds infrastructure for keyboard led status tracking to qemu.
> >
> > Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> 
> Applied.  Thanks.

What about guests that use the capslock LED for something useful, instead of 
capslock?

Paul
Anthony Liguori - March 9, 2010, 4:09 p.m.
On 03/09/2010 09:58 AM, Paul Brook wrote:
>> On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
>>      
>>> Adds infrastructure for keyboard led status tracking to qemu.
>>>
>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>>        
>> Applied.  Thanks.
>>      
> What about guests that use the capslock LED for something useful, instead of
> capslock?
>    

For VNC, tracking modifier state is already a heuristic so this series 
just adjusts that heuristic in a way that is better for 99% of use-cases.

I agree that we have to be careful about how we use information like 
this particularly when it comes to issues of correctness, but I don't 
believe this is such an instance.


Regards,

Anthony Liguori

> Paul
>
Gerd Hoffmann - March 9, 2010, 4:13 p.m.
On 03/09/10 16:58, Paul Brook wrote:
>> On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
>>> Adds infrastructure for keyboard led status tracking to qemu.
>>>
>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>
>> Applied.  Thanks.
>
> What about guests that use the capslock LED for something useful, instead of
> capslock?

I offered write add a patch with a option to this off if you think it is 
not needed.  Never got an answer, so I didn't.  Patch is on the list now.

cheers,
   Gerd
Anthony Liguori - March 9, 2010, 4:28 p.m.
On 03/09/2010 10:13 AM, Gerd Hoffmann wrote:
> On 03/09/10 16:58, Paul Brook wrote:
>>> On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
>>>> Adds infrastructure for keyboard led status tracking to qemu.
>>>>
>>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>>
>>> Applied.  Thanks.
>>
>> What about guests that use the capslock LED for something useful, 
>> instead of
>> capslock?
>
> I offered write add a patch with a option to this off if you think it 
> is not needed.  Never got an answer, so I didn't.  Patch is on the 
> list now.

If a person has a guest that twiddles with LEDs, what are the chances 
they are going to figure out that there's an option with VNC to not use 
LED status as part of the modifier tracking heuristics?

I don't think it's really useful.  Like I said in an earlier note, we're 
already dealing with a heuristics based algorithm because we're 
attempting to detect dangling modifier presses.  I'd rather see an 
option that disabled the whole thing verses something specific to the LEDs.

Regards,

Anthony Liguori

> cheers,
>   Gerd
Gerd Hoffmann - March 9, 2010, 4:41 p.m.
On 03/09/10 17:28, Anthony Liguori wrote:
> If a person has a guest that twiddles with LEDs, what are the chances
> they are going to figure out that there's an option with VNC to not use
> LED status as part of the modifier tracking heuristics?
>
> I don't think it's really useful. Like I said in an earlier note, we're
> already dealing with a heuristics based algorithm because we're
> attempting to detect dangling modifier presses. I'd rather see an option
> that disabled the whole thing verses something specific to the LEDs.

You are referring to the 
do-i-should-insert-extra-capslock/numlock-events logic in do_key_event() 
?  Well, we can turn off both, fine with me, after all they sort-of 
belong together.  Do you have an idea for a short 
'no-capslock-and-numlock-tracking-heuristics' option name?

cheers,
   Gerd
Anthony Liguori - March 9, 2010, 5:10 p.m.
On 03/09/2010 10:41 AM, Gerd Hoffmann wrote:
> On 03/09/10 17:28, Anthony Liguori wrote:
>> If a person has a guest that twiddles with LEDs, what are the chances
>> they are going to figure out that there's an option with VNC to not use
>> LED status as part of the modifier tracking heuristics?
>>
>> I don't think it's really useful. Like I said in an earlier note, we're
>> already dealing with a heuristics based algorithm because we're
>> attempting to detect dangling modifier presses. I'd rather see an option
>> that disabled the whole thing verses something specific to the LEDs.
>
> You are referring to the 
> do-i-should-insert-extra-capslock/numlock-events logic in do_key_event() ?

Yup.

>   Well, we can turn off both, fine with me, after all they sort-of 
> belong together.  Do you have an idea for a short 
> 'no-capslock-and-numlock-tracking-heuristics' option name?

Maybe 'toggle-key-tracking=on|off' with an appropriate documentation 
blurb.  I don't know, it doesn't sound like a good name to me.

Regards,

Anthony Liguori


>
> cheers,
>   Gerd
>

Patch

diff --git a/console.h b/console.h
index 916859d..71e8ff2 100644
--- a/console.h
+++ b/console.h
@@ -10,10 +10,16 @@ 
 #define MOUSE_EVENT_RBUTTON 0x02
 #define MOUSE_EVENT_MBUTTON 0x04
 
+/* identical to the ps/2 keyboard bits */
+#define QEMU_SCROLL_LOCK_LED (1 << 0)
+#define QEMU_NUM_LOCK_LED    (1 << 1)
+#define QEMU_CAPS_LOCK_LED   (1 << 2)
+
 /* in ms */
 #define GUI_REFRESH_INTERVAL 30
 
 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 {
@@ -26,13 +32,22 @@  typedef struct QEMUPutMouseEntry {
     struct QEMUPutMouseEntry *next;
 } QEMUPutMouseEntry;
 
+typedef struct QEMUPutLEDEntry {
+    QEMUPutLEDEvent *put_led;
+    void *opaque;
+    QTAILQ_ENTRY(QEMUPutLEDEntry) next;
+} QEMUPutLEDEntry;
+
 void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
 QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name);
 void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry);
+QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, void *opaque);
+void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
 
 void kbd_put_keycode(int keycode);
+void kbd_put_ledstate(int ledstate);
 void kbd_mouse_event(int dx, int dy, int dz, int buttons_state);
 int kbd_mouse_is_absolute(void);
 
diff --git a/input.c b/input.c
index 955b9ab..baaa4c6 100644
--- a/input.c
+++ b/input.c
@@ -33,6 +33,7 @@  static QEMUPutKBDEvent *qemu_put_kbd_event;
 static void *qemu_put_kbd_event_opaque;
 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)
 {
@@ -102,6 +103,27 @@  void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry)
     qemu_free(entry);
 }
 
+QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func,
+                                            void *opaque)
+{
+    QEMUPutLEDEntry *s;
+
+    s = qemu_mallocz(sizeof(QEMUPutLEDEntry));
+
+    s->put_led = func;
+    s->opaque = opaque;
+    QTAILQ_INSERT_TAIL(&led_handlers, s, next);
+    return s;
+}
+
+void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
+{
+    if (entry == NULL)
+        return;
+    QTAILQ_REMOVE(&led_handlers, entry, next);
+    qemu_free(entry);
+}
+
 void kbd_put_keycode(int keycode)
 {
     if (qemu_put_kbd_event) {
@@ -109,6 +131,15 @@  void kbd_put_keycode(int keycode)
     }
 }
 
+void kbd_put_ledstate(int ledstate)
+{
+    QEMUPutLEDEntry *cursor;
+
+    QTAILQ_FOREACH(cursor, &led_handlers, next) {
+        cursor->put_led(cursor->opaque, ledstate);
+    }
+}
+
 void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
 {
     QEMUPutMouseEvent *mouse_event;