Patchwork [4/4] kbd keds: vnc

login
register
mail settings
Submitter Gerd Hoffmann
Date Feb. 25, 2010, 8:39 a.m.
Message ID <1267087161-15204-5-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/46222/
State New
Headers show

Comments

Gerd Hoffmann - Feb. 25, 2010, 8:39 a.m.
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(-)
Juan Quintela - Feb. 25, 2010, 11:03 a.m.
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.
Paolo Bonzini - Feb. 25, 2010, 11:11 a.m.
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
Gerd Hoffmann - Feb. 26, 2010, 4:05 p.m.
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
Paul Brook - Feb. 28, 2010, 1:38 a.m.
> 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
Gerd Hoffmann - March 1, 2010, 8:12 a.m.
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

Patch

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;