[Qemu-devel] [PATCH 0/4] keyboard led status tracking

[Qemu-devel] [PATCH 0/4] keyboard led status tracking

[Gerd Hoffmann]

  Hi,

This patch series adds support for keyboard led status tracking to qemu.
The first patch adds the infrastructure to set the led status and to
register notify callbacks.  Patches #2 and #3 add support for the usb
and ps/2 keyboard drivers.  Patch #4 makes vnc use the led notification
to improve capslock and numlock status tracking.  spice will use the
kbd led notifiers too.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/4] kbd leds: infrastructure

[Gerd Hoffmann]

Adds infrastructure for keyboard led status tracking to qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 console.h |   14 ++++++++++++++
 input.c   |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/console.h b/console.h
index 916859d..0e969d1 100644
--- a/console.h
+++ b/console.h
@@ -10,10 +10,15 @@
 #define MOUSE_EVENT_RBUTTON 0x02
 #define MOUSE_EVENT_MBUTTON 0x04
 
+#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 +31,22 @@ typedef struct QEMUPutMouseEntry {
     struct QEMUPutMouseEntry *next;
 } QEMUPutMouseEntry;
 
+typedef struct QEMUPutLEDEntry {
+    QEMUPutLEDEvent *put_led;
+    void *opaque;
+    struct 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..82bc85c 100644
--- a/input.c
+++ b/input.c
@@ -31,6 +31,7 @@
 
 static QEMUPutKBDEvent *qemu_put_kbd_event;
 static void *qemu_put_kbd_event_opaque;
+static QEMUPutLEDEntry *qemu_put_led_event_head;
 static QEMUPutMouseEntry *qemu_put_mouse_event_head;
 static QEMUPutMouseEntry *qemu_put_mouse_event_current;
 
@@ -102,6 +103,44 @@ 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;
+    s->next = qemu_put_led_event_head;
+    qemu_put_led_event_head = s;
+    return s;
+}
+
+void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
+{
+    QEMUPutLEDEntry *prev = NULL, *cursor;
+
+    if (!qemu_put_led_event_head || entry == NULL)
+        return;
+
+    cursor = qemu_put_led_event_head;
+    while (cursor != NULL && cursor != entry) {
+        prev = cursor;
+        cursor = cursor->next;
+    }
+
+    if (cursor == NULL) // does not exist or list empty
+        return;
+
+    if (prev == NULL) { // entry is head
+        qemu_put_led_event_head = cursor->next;
+    } else {
+        prev->next = entry->next;
+    }
+    qemu_free(entry);
+}
+
 void kbd_put_keycode(int keycode)
 {
     if (qemu_put_kbd_event) {
@@ -109,6 +148,17 @@ void kbd_put_keycode(int keycode)
     }
 }
 
+void kbd_put_ledstate(int ledstate)
+{
+    QEMUPutLEDEntry *cursor;
+
+    cursor = qemu_put_led_event_head;
+    while (cursor != NULL) {
+        cursor->put_led(cursor->opaque, ledstate);
+        cursor = cursor->next;
+    }
+}
+
 void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
 {
     QEMUPutMouseEvent *mouse_event;
-- 
1.6.6

[Qemu-devel] Re: [PATCH 1/4] kbd leds: infrastructure

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
> Adds infrastructure for keyboard led status tracking to qemu.
>  } QEMUPutMouseEntry;
>  
> +typedef struct QEMUPutLEDEntry {
> +    QEMUPutLEDEvent *put_led;
> +    void *opaque;
> +    struct QEMUPutLEDEntry *next;
> +} QEMUPutLEDEntry;

Please, change this to a QLIST(), code becomes way simpler.
> +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
> +{
> +    QEMUPutLEDEntry *prev = NULL, *cursor;
> +
> +    if (!qemu_put_led_event_head || entry == NULL)
> +        return;
> +
> +    cursor = qemu_put_led_event_head;
> +    while (cursor != NULL && cursor != entry) {
> +        prev = cursor;
> +        cursor = cursor->next;
> +    }
> +
> +    if (cursor == NULL) // does not exist or list empty
> +        return;
> +
> +    if (prev == NULL) { // entry is head
> +        qemu_put_led_event_head = cursor->next;
> +    } else {
> +        prev->next = entry->next;
> +    }
> +    qemu_free(entry);
> +}

This functions simplifies to:

void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
{
    QLIST_REMOVE(entry, leds);
    qemu_free(entry);
}

Rest of gains are smaller, but easier to read.

Concept of the patch is ok with me.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/4] kbd leds: infrastructure

[Anthony Liguori]

On 02/25/2010 02:39 AM, Gerd Hoffmann wrote:
> Adds infrastructure for keyboard led status tracking to qemu.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>    

This is an obvious extension to the current API so I'm not necessarily 
opposed to it.

But I wonder if it really makes sense to treat all of these things 
differently since we end up duplicating a lot of code.  Would it make 
more sense to just introduce:

typedef struct QEMUInputHandler {
    void (*put_kbd_event)(QEMUInputHandler *obj, int keycode);
    void (*put_led_event)(QEMUInputHandler *obj, int ledstate);
    void (*put_mouse_event)(QEMUInputHandler *obj, int dx, int dy, int 
dz, int buttons_state);
    QLIST_ENTRY(QEMUInputHandler) node;
} QEMUInputHandler;

void qemu_add_input_handler(QEMUInputHandler *handler);
void qemu_remove_input_handler(QEMUInputHandler *handler);

Regards,

Anthony Liguori

> ---
>   console.h |   14 ++++++++++++++
>   input.c   |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 64 insertions(+), 0 deletions(-)
>
> diff --git a/console.h b/console.h
> index 916859d..0e969d1 100644
> --- a/console.h
> +++ b/console.h
> @@ -10,10 +10,15 @@
>   #define MOUSE_EVENT_RBUTTON 0x02
>   #define MOUSE_EVENT_MBUTTON 0x04
>
> +#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 +31,22 @@ typedef struct QEMUPutMouseEntry {
>       struct QEMUPutMouseEntry *next;
>   } QEMUPutMouseEntry;
>
> +typedef struct QEMUPutLEDEntry {
> +    QEMUPutLEDEvent *put_led;
> +    void *opaque;
> +    struct 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..82bc85c 100644
> --- a/input.c
> +++ b/input.c
> @@ -31,6 +31,7 @@
>
>   static QEMUPutKBDEvent *qemu_put_kbd_event;
>   static void *qemu_put_kbd_event_opaque;
> +static QEMUPutLEDEntry *qemu_put_led_event_head;
>   static QEMUPutMouseEntry *qemu_put_mouse_event_head;
>   static QEMUPutMouseEntry *qemu_put_mouse_event_current;
>
> @@ -102,6 +103,44 @@ 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;
> +    s->next = qemu_put_led_event_head;
> +    qemu_put_led_event_head = s;
> +    return s;
> +}
> +
> +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
> +{
> +    QEMUPutLEDEntry *prev = NULL, *cursor;
> +
> +    if (!qemu_put_led_event_head || entry == NULL)
> +        return;
> +
> +    cursor = qemu_put_led_event_head;
> +    while (cursor != NULL&&  cursor != entry) {
> +        prev = cursor;
> +        cursor = cursor->next;
> +    }
> +
> +    if (cursor == NULL) // does not exist or list empty
> +        return;
> +
> +    if (prev == NULL) { // entry is head
> +        qemu_put_led_event_head = cursor->next;
> +    } else {
> +        prev->next = entry->next;
> +    }
> +    qemu_free(entry);
> +}
> +
>   void kbd_put_keycode(int keycode)
>   {
>       if (qemu_put_kbd_event) {
> @@ -109,6 +148,17 @@ void kbd_put_keycode(int keycode)
>       }
>   }
>
> +void kbd_put_ledstate(int ledstate)
> +{
> +    QEMUPutLEDEntry *cursor;
> +
> +    cursor = qemu_put_led_event_head;
> +    while (cursor != NULL) {
> +        cursor->put_led(cursor->opaque, ledstate);
> +        cursor = cursor->next;
> +    }
> +}
> +
>   void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
>   {
>       QEMUPutMouseEvent *mouse_event;
>    

Re: [Qemu-devel] [PATCH 1/4] kbd leds: infrastructure

[Gerd Hoffmann]

On 02/25/10 15:15, Anthony Liguori wrote:
> But I wonder if it really makes sense to treat all of these things
> differently since we end up duplicating a lot of code. Would it make
> more sense to just introduce:

> typedef struct QEMUInputHandler {
> void (*put_kbd_event)(QEMUInputHandler *obj, int keycode);
> void (*put_led_event)(QEMUInputHandler *obj, int ledstate);
> void (*put_mouse_event)(QEMUInputHandler *obj, int dx, int dy, int dz,
> int buttons_state);
> QLIST_ENTRY(QEMUInputHandler) node;
> } QEMUInputHandler;

> void qemu_add_input_handler(QEMUInputHandler *handler);
> void qemu_remove_input_handler(QEMUInputHandler *handler);

I don't think so.  Devil is in the details.  Note that kbd_event and 
mouse_event go to the kbd/mouse drivers, whereas led_event comes from 
the kbd driver, so they are different albeit related beasts.  Also the 
mouse register/unregister does some extra care to update the pointer to 
the current mouse device, so we can't easily unify the list management.

Juan's suggestion to use QLISTs makes alot of sense though, will do that.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/4] kbd leds: infrastructure

[Anthony Liguori]

On 02/25/2010 08:36 AM, Gerd Hoffmann wrote:
> On 02/25/10 15:15, Anthony Liguori wrote:
>> But I wonder if it really makes sense to treat all of these things
>> differently since we end up duplicating a lot of code. Would it make
>> more sense to just introduce:
>
>> typedef struct QEMUInputHandler {
>> void (*put_kbd_event)(QEMUInputHandler *obj, int keycode);
>> void (*put_led_event)(QEMUInputHandler *obj, int ledstate);
>> void (*put_mouse_event)(QEMUInputHandler *obj, int dx, int dy, int dz,
>> int buttons_state);
>> QLIST_ENTRY(QEMUInputHandler) node;
>> } QEMUInputHandler;
>
>> void qemu_add_input_handler(QEMUInputHandler *handler);
>> void qemu_remove_input_handler(QEMUInputHandler *handler);
>
> I don't think so.  Devil is in the details.  Note that kbd_event and 
> mouse_event go to the kbd/mouse drivers, whereas led_event comes from 
> the kbd driver, so they are different albeit related beasts. 

Right, I hadn't thought it through enough.

Should led events be part of DisplayChangeListener?  I think you could 
make the argument that it's part of the display state.

Regards,

Anthony Liguori


> Also the mouse register/unregister does some extra care to update the 
> pointer to the current mouse device, so we can't easily unify the list 
> management.
>
> Juan's suggestion to use QLISTs makes alot of sense though, will do that.
>
> cheers,
>   Gerd
>

Re: [Qemu-devel] [PATCH 1/4] kbd leds: infrastructure

[Gerd Hoffmann]

On 02/25/10 16:07, Anthony Liguori wrote:
> On 02/25/2010 08:36 AM, Gerd Hoffmann wrote:
>> I don't think so. Devil is in the details. Note that kbd_event and
>> mouse_event go to the kbd/mouse drivers, whereas led_event comes from
>> the kbd driver, so they are different albeit related beasts.
>
> Right, I hadn't thought it through enough.
>
> Should led events be part of DisplayChangeListener? I think you could
> make the argument that it's part of the display state.

 From a hardware point of view display and keyboard are not related at 
all.  The keyboard drivers don't have a DisplayState pointer at hand for 
that reason.  So I don't think this makes sense, even though remote 
desktop protocols typically transport both display and keyboard data.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/4] kbd leds: infrastructure

[Anthony Liguori]

On 02/25/2010 10:51 AM, Gerd Hoffmann wrote:
> On 02/25/10 16:07, Anthony Liguori wrote:
>> On 02/25/2010 08:36 AM, Gerd Hoffmann wrote:
>>> I don't think so. Devil is in the details. Note that kbd_event and
>>> mouse_event go to the kbd/mouse drivers, whereas led_event comes from
>>> the kbd driver, so they are different albeit related beasts.
>>
>> Right, I hadn't thought it through enough.
>>
>> Should led events be part of DisplayChangeListener? I think you could
>> make the argument that it's part of the display state.
>
> From a hardware point of view display and keyboard are not related at 
> all.  The keyboard drivers don't have a DisplayState pointer at hand 
> for that reason.  So I don't think this makes sense, even though 
> remote desktop protocols typically transport both display and keyboard 
> data.

Fair enough.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

[Qemu-devel] [PATCH 2/4] kbd leds: ps/2 kbd

[Gerd Hoffmann]

Add led status notification support to the ps/2 kbd driver.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ps2.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/ps2.c b/hw/ps2.c
index 15a6650..3dc3b15 100644
--- a/hw/ps2.c
+++ b/hw/ps2.c
@@ -185,6 +185,7 @@ static void ps2_reset_keyboard(PS2KbdState *s)
 {
     s->scan_enabled = 1;
     s->scancode_set = 2;
+    kbd_put_ledstate(0);
 }
 
 void ps2_write_keyboard(void *opaque, int val)
@@ -259,6 +260,7 @@ void ps2_write_keyboard(void *opaque, int val)
         s->common.write_cmd = -1;
         break;
     case KBD_CMD_SET_LEDS:
+        kbd_put_ledstate(val);
         ps2_queue(&s->common, KBD_REPLY_ACK);
         s->common.write_cmd = -1;
         break;
-- 
1.6.6

[Qemu-devel] Re: [PATCH 2/4] kbd leds: ps/2 kbd

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
> Add led status notification support to the ps/2 kbd driver.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/ps2.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/hw/ps2.c b/hw/ps2.c
> index 15a6650..3dc3b15 100644
> --- a/hw/ps2.c
> +++ b/hw/ps2.c
> @@ -185,6 +185,7 @@ static void ps2_reset_keyboard(PS2KbdState *s)
>  {
>      s->scan_enabled = 1;
>      s->scancode_set = 2;
> +    kbd_put_ledstate(0);
>  }
>  
>  void ps2_write_keyboard(void *opaque, int val)
> @@ -259,6 +260,7 @@ void ps2_write_keyboard(void *opaque, int val)
>          s->common.write_cmd = -1;
>          break;
>      case KBD_CMD_SET_LEDS:
> +        kbd_put_ledstate(val);

You need a comment here, stating that the new QEMU_*_LOCK_LED values are the same
than the ps2 ones.

>          ps2_queue(&s->common, KBD_REPLY_ACK);
>          s->common.write_cmd = -1;
>          break;

Agreed with the rest of the changes.

[Qemu-devel] [PATCH 3/4] kbd leds: usb kbd

[Gerd Hoffmann]

Add led status notification support to the usb kbd driver.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-hid.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/usb-hid.c b/hw/usb-hid.c
index bf456bb..2e4e647 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -600,12 +600,20 @@ static int usb_keyboard_poll(USBKeyboardState *s, uint8_t *buf, int len)
 static int usb_keyboard_write(USBKeyboardState *s, uint8_t *buf, int len)
 {
     if (len > 0) {
+        int ledstate = 0;
         /* 0x01: Num Lock LED
          * 0x02: Caps Lock LED
          * 0x04: Scroll Lock LED
          * 0x08: Compose LED
          * 0x10: Kana LED */
         s->leds = buf[0];
+        if (s->leds & 0x04)
+            ledstate |= QEMU_SCROLL_LOCK_LED;
+        if (s->leds & 0x01)
+            ledstate |= QEMU_NUM_LOCK_LED;
+        if (s->leds & 0x02)
+            ledstate |= QEMU_CAPS_LOCK_LED;
+        kbd_put_ledstate(ledstate);
     }
     return 0;
 }
-- 
1.6.6

[Qemu-devel] [PATCH 4/4] kbd keds: vnc

[Gerd Hoffmann]

Use led status notification support in vnc.

The qemu vnc server keeps track of the capslock and numlock states based
on the key presses it receives from the vnc client.  But this fails in
case the guests idea of the capslock and numlock state changes for other
reasons.  One case is guest reboot (+ keyboard reset).  Another case are
more recent windows versions which reset capslock state before
presenting the login screen.

Usually guests use the keyboard leds to signal the capslock and numlock
state to the user, so we can use this to better keep track of capslock
and numlock state in the qemu vnc server.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vnc.c |   20 +++++++++++++++++++-
 vnc.h |    1 +
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/vnc.c b/vnc.c
index db34b0e..38690e2 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1111,6 +1111,7 @@ static void vnc_disconnect_finish(VncState *vs)
     }
 
     vnc_remove_timer(vs->vd);
+    qemu_remove_led_event_handler(vs->led);
     qemu_free(vs);
 }
 
@@ -1496,6 +1497,22 @@ static void press_key(VncState *vs, int keysym)
     kbd_put_keycode(keysym2scancode(vs->vd->kbd_layout, keysym) | 0x80);
 }
 
+static void kbd_leds(void *opaque, int ledstate)
+{
+    VncState *vs = opaque;
+    int caps, num;
+
+    caps = ledstate & QEMU_CAPS_LOCK_LED ? 1 : 0;
+    num  = ledstate & QEMU_NUM_LOCK_LED  ? 1 : 0;
+
+    if (vs->modifiers_state[0x3a] != caps) {
+        vs->modifiers_state[0x3a] = caps;
+    }
+    if (vs->modifiers_state[0x45] != num) {
+        vs->modifiers_state[0x45] = num;
+    }
+}
+
 static void do_key_event(VncState *vs, int down, int keycode, int sym)
 {
     /* QEMU console switch */
@@ -1521,7 +1538,7 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
         break;
     case 0x3a:                        /* CapsLock */
     case 0x45:                        /* NumLock */
-        if (!down)
+        if (down)
             vs->modifiers_state[keycode] ^= 1;
         break;
     }
@@ -2407,6 +2424,7 @@ static void vnc_connect(VncDisplay *vd, int csock)
     vnc_flush(vs);
     vnc_read_when(vs, protocol_version, 12);
     reset_keys(vs);
+    vs->led = qemu_add_led_event_handler(kbd_leds, vs);
 
     vnc_init_timer(vd);
 
diff --git a/vnc.h b/vnc.h
index ff9a699..0fc89bd 100644
--- a/vnc.h
+++ b/vnc.h
@@ -161,6 +161,7 @@ struct VncState
     size_t read_handler_expect;
     /* input */
     uint8_t modifiers_state[256];
+    QEMUPutLEDEntry *led;
 
     Buffer zlib;
     Buffer zlib_tmp;
-- 
1.6.6

[Qemu-devel] Re: [PATCH 4/4] kbd keds: vnc

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
> Use led status notification support in vnc.

> +static void kbd_leds(void *opaque, int ledstate)
> +{
> +    VncState *vs = opaque;
> +    int caps, num;
> +
> +    caps = ledstate & QEMU_CAPS_LOCK_LED ? 1 : 0;
> +    num  = ledstate & QEMU_NUM_LOCK_LED  ? 1 : 0;

I think it is clearer to use a bool.

   bool caps = ledstate & QEMU_CAPS_LOCK_LED;


> +    if (vs->modifiers_state[0x3a] != caps) {
> +        vs->modifiers_state[0x3a] = caps;

modifiers_state type needs to go from uint8_t to bool.  It simplifies
lots of !!foo around.  But the change is independent of this series.

> +    }
> +    if (vs->modifiers_state[0x45] != num) {
> +        vs->modifiers_state[0x45] = num;
> +    }
> +}
> +
>  static void do_key_event(VncState *vs, int down, int keycode, int sym)
>  {
>      /* QEMU console switch */
> @@ -1521,7 +1538,7 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
>          break;
>      case 0x3a:                        /* CapsLock */
>      case 0x45:                        /* NumLock */
> -        if (!down)
> +        if (down)
>              vs->modifiers_state[keycode] ^= 1;
>          break;
>      }

This needs a comment on the changelog why this is needed IMHO.

Later, Juan.

[Qemu-devel] Re: [PATCH 4/4] kbd keds: vnc

[Paolo Bonzini]

On 02/25/2010 12:03 PM, Juan Quintela wrote:
>> >  +    caps = ledstate & QEMU_CAPS_LOCK_LED ? 1 : 0;
>> >  +    num  = ledstate & QEMU_NUM_LOCK_LED  ? 1 : 0;
> I think it is clearer to use a bool.
>
>     bool caps = ledstate&  QEMU_CAPS_LOCK_LED;

Are we assuming a C99 bool elsewhere?  For example, I see only uses of 
bool like

hw/eepro100.c:        bool bit_el = ((command & 0x8000) != 0);

So yet another choice is

   caps = (ledstate & QEMU_CAPS_LOCK_LED) != 0;

Paolo

[Qemu-devel] Re: [PATCH 4/4] kbd keds: vnc

[Gerd Hoffmann]

   Hi,

>> +    int caps, num;
>
> I think it is clearer to use a bool.
>
>     bool caps = ledstate&  QEMU_CAPS_LOCK_LED;

Indeed.

>> +    if (vs->modifiers_state[0x3a] != caps) {
>> +        vs->modifiers_state[0x3a] = caps;
>
> modifiers_state type needs to go from uint8_t to bool.  It simplifies
> lots of !!foo around.  But the change is independent of this series.

But mixing uin8_t and bool doesn't look sane to me.  So I think I stay 
with uin8_t to match modifiers_state type.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/4] kbd keds: vnc

[Paul Brook]

> Use led status notification support in vnc.
> 
> The qemu vnc server keeps track of the capslock and numlock states based
> on the key presses it receives from the vnc client.  But this fails in
> case the guests idea of the capslock and numlock state changes for other
> reasons.  One case is guest reboot (+ keyboard reset).  Another case are
> more recent windows versions which reset capslock state before
> presenting the login screen.
> 
> Usually guests use the keyboard leds to signal the capslock and numlock
> state to the user, so we can use this to better keep track of capslock
> and numlock state in the qemu vnc server.

Isn't this going to break horribly when my guest starts flashing the capslock 
light in response to network traffic?

Paul

Re: [Qemu-devel] [PATCH 4/4] kbd keds: vnc

[Gerd Hoffmann]

On 02/28/10 02:38, Paul Brook wrote:
>> Usually guests use the keyboard leds to signal the capslock and numlock
>> state to the user, so we can use this to better keep track of capslock
>> and numlock state in the qemu vnc server.
>
> Isn't this going to break horribly when my guest starts flashing the capslock
> light in response to network traffic?

Yes, if the guest plays blinkenlights with the keyboard leds this 
breaks.  I've considered adding option to turn this off, but then 
decided to wait and see whenever it actually is a problem in practice.

The only common case I'm aware of is the linux kernel panic blinking.

I'll add a option to turn this off if you think we'll need one.

cheers,
   Gerd