diff mbox

vnc: track LED state separately

Message ID 3fcc534530370027118377dc24d4049a1cef590e.1481807806.git.ossman@cendio.se
State New
Headers show

Commit Message

Pierre Ossman Dec. 15, 2016, 1:16 p.m. UTC 
Piggy-backing on the modifier state array made it difficult to send
out updates at the proper times. So keep track of the LED state in
a separate variable.

This also moves the handling up a layer in to the VNC Display object
since the state is global, and also makes sure the state is readily
available directly when a client connects.

Signed-off-by: Pierre Ossman <ossman@cendio.se>
---
 ui/vnc.c | 56 +++++++++++++-------------------------------------------
 ui/vnc.h |  3 ++-
 2 files changed, 15 insertions(+), 44 deletions(-)

Comments

Gerd Hoffmann Dec. 15, 2016, 4:41 p.m. UTC | #1 
On Do, 2016-12-15 at 14:16 +0100, Pierre Ossman wrote:
> Piggy-backing on the modifier state array made it difficult to send
> out updates at the proper times. So keep track of the LED state in
> a separate variable.
> 
> This also moves the handling up a layer in to the VNC Display object
> since the state is global, and also makes sure the state is readily
> available directly when a client connects.

Better, thanks.

>  static void kbd_leds(void *opaque, int ledstate)
>  {
> -    VncState *vs = opaque;
> -    int caps, num, scr;
> -    bool has_changed = (ledstate != current_led_state(vs));
> +    VncDisplay *vd = opaque;
> +    VncState *client;
>  
>      trace_vnc_key_guest_leds((ledstate & QEMU_CAPS_LOCK_LED),
>                               (ledstate & QEMU_NUM_LOCK_LED),
>                               (ledstate & QEMU_SCROLL_LOCK_LED));
>  
> -    caps = ledstate & QEMU_CAPS_LOCK_LED ? 1 : 0;
> -    num  = ledstate & QEMU_NUM_LOCK_LED  ? 1 : 0;
> -    scr  = ledstate & QEMU_SCROLL_LOCK_LED ? 1 : 0;
> +    if (ledstate != vd->ledstate)
> +        return;

Hmm?  Shouldn't that be (ledstate == vd->ledstate)?

cheers,
  Gerd
Pierre Ossman Dec. 16, 2016, 9 a.m. UTC | #2 
On 15/12/16 17:41, Gerd Hoffmann wrote:
>>
>> -    caps = ledstate & QEMU_CAPS_LOCK_LED ? 1 : 0;
>> -    num  = ledstate & QEMU_NUM_LOCK_LED  ? 1 : 0;
>> -    scr  = ledstate & QEMU_SCROLL_LOCK_LED ? 1 : 0;
>> +    if (ledstate != vd->ledstate)
>> +        return;
>
> Hmm?  Shouldn't that be (ledstate == vd->ledstate)?
>

Right, sorry. Brain fart. :)

Can you fix that up before commit or do you want a new patch?

Regards
Gerd Hoffmann Jan. 4, 2017, 8:57 a.m. UTC | #3 
On Fr, 2016-12-16 at 10:00 +0100, Pierre Ossman wrote:
> On 15/12/16 17:41, Gerd Hoffmann wrote:
> >>
> >> -    caps = ledstate & QEMU_CAPS_LOCK_LED ? 1 : 0;
> >> -    num  = ledstate & QEMU_NUM_LOCK_LED  ? 1 : 0;
> >> -    scr  = ledstate & QEMU_SCROLL_LOCK_LED ? 1 : 0;
> >> +    if (ledstate != vd->ledstate)
> >> +        return;
> >
> > Hmm?  Shouldn't that be (ledstate == vd->ledstate)?
> >
> 
> Right, sorry. Brain fart. :)

/me wonders how you've tested the patch ...

> Can you fix that up before commit or do you want a new patch?

Tried, but checkpatch found some more issues:

=== checkpatch complains ===
ERROR: braces {} are necessary for all arms of this statement
#71: FILE: ui/vnc.c:1691:
+    if (ledstate != vd->ledstate)
[...]

ERROR: braces {} are necessary for all arms of this statement
#106: FILE: ui/vnc.c:3159:
+    if (vd->lock_key_sync)
[...]

ERROR: braces {} are necessary for all arms of this statement
#115: FILE: ui/vnc.c:3728:
+    if (lock_key_sync)
[...]

total: 3 errors, 0 warnings, 125 lines checked

Please fix and resend.

thanks,
  Gerd

> 
> Regards
Pierre Ossman Jan. 9, 2017, 4:14 p.m. UTC | #4 
On 04/01/17 09:57, Gerd Hoffmann wrote:
>
> /me wonders how you've tested the patch ...
>

That would be not at all. Or rather, just a compile. :)

>
> Tried, but checkpatch found some more issues:
>

Fixed. New patch incoming.

Regards
diff mbox

Patch

diff --git a/ui/vnc.c b/ui/vnc.c
index 2c28a59..c449a7d 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1231,8 +1231,6 @@  void vnc_disconnect_finish(VncState *vs)
         vnc_update_server_surface(vs->vd);
     }
 
-    if (vs->vd->lock_key_sync)
-        qemu_remove_led_event_handler(vs->led);
     vnc_unlock_output(vs);
 
     qemu_mutex_destroy(&vs->output_mutex);
@@ -1665,69 +1663,38 @@  static void press_key(VncState *vs, int keysym)
     qemu_input_event_send_key_delay(vs->vd->key_delay_ms);
 }
 
-static int current_led_state(VncState *vs)
-{
-    int ledstate = 0;
-
-    if (vs->modifiers_state[0x46]) {
-        ledstate |= QEMU_SCROLL_LOCK_LED;
-    }
-    if (vs->modifiers_state[0x45]) {
-        ledstate |= QEMU_NUM_LOCK_LED;
-    }
-    if (vs->modifiers_state[0x3a]) {
-        ledstate |= QEMU_CAPS_LOCK_LED;
-    }
-
-    return ledstate;
-}
-
 static void vnc_led_state_change(VncState *vs)
 {
-    int ledstate = 0;
-
     if (!vnc_has_feature(vs, VNC_FEATURE_LED_STATE)) {
         return;
     }
 
-    ledstate = current_led_state(vs);
     vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
     vnc_write_u16(vs, 1);
     vnc_framebuffer_update(vs, 0, 0, 1, 1, VNC_ENCODING_LED_STATE);
-    vnc_write_u8(vs, ledstate);
+    vnc_write_u8(vs, vs->vd->ledstate);
     vnc_unlock_output(vs);
     vnc_flush(vs);
 }
 
 static void kbd_leds(void *opaque, int ledstate)
 {
-    VncState *vs = opaque;
-    int caps, num, scr;
-    bool has_changed = (ledstate != current_led_state(vs));
+    VncDisplay *vd = opaque;
+    VncState *client;
 
     trace_vnc_key_guest_leds((ledstate & QEMU_CAPS_LOCK_LED),
                              (ledstate & QEMU_NUM_LOCK_LED),
                              (ledstate & QEMU_SCROLL_LOCK_LED));
 
-    caps = ledstate & QEMU_CAPS_LOCK_LED ? 1 : 0;
-    num  = ledstate & QEMU_NUM_LOCK_LED  ? 1 : 0;
-    scr  = ledstate & QEMU_SCROLL_LOCK_LED ? 1 : 0;
+    if (ledstate != vd->ledstate)
+        return;
 
-    if (vs->modifiers_state[0x3a] != caps) {
-        vs->modifiers_state[0x3a] = caps;
-    }
-    if (vs->modifiers_state[0x45] != num) {
-        vs->modifiers_state[0x45] = num;
-    }
-    if (vs->modifiers_state[0x46] != scr) {
-        vs->modifiers_state[0x46] = scr;
-    }
+    vd->ledstate = ledstate;
 
-    /* Sending the current led state message to the client */
-    if (has_changed) {
-        vnc_led_state_change(vs);
+    QTAILQ_FOREACH(client, &vd->clients, next) {
+        vnc_led_state_change(client);
     }
 }
 
@@ -3083,8 +3050,6 @@  void vnc_start_protocol(VncState *vs)
     vnc_write(vs, "RFB 003.008\n", 12);
     vnc_flush(vs);
     vnc_read_when(vs, protocol_version, 12);
-    if (vs->vd->lock_key_sync)
-        vs->led = qemu_add_led_event_handler(kbd_leds, vs);
 
     vs->mouse_mode_notifier.notify = check_pointer_type_change;
     qemu_add_mouse_mode_change_notifier(&vs->mouse_mode_notifier);
@@ -3191,6 +3156,8 @@  static void vnc_display_close(VncDisplay *vd)
     }
     g_free(vd->tlsaclname);
     vd->tlsaclname = NULL;
+    if (vd->lock_key_sync)
+        qemu_remove_led_event_handler(vd->led);
 }
 
 int vnc_display_password(const char *id, const char *password)
@@ -3758,6 +3725,9 @@  void vnc_display_open(const char *id, Error **errp)
     }
 #endif
     vd->lock_key_sync = lock_key_sync;
+    if (lock_key_sync)
+        vd->led = qemu_add_led_event_handler(kbd_leds, vd);
+    vd->ledstate = 0;
     vd->key_delay_ms = key_delay_ms;
 
     device_id = qemu_opt_get(opts, "display");
diff --git a/ui/vnc.h b/ui/vnc.h
index d20b154..d8c9de5 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -154,6 +154,8 @@  struct VncDisplay
     DisplayChangeListener dcl;
     kbd_layout_t *kbd_layout;
     int lock_key_sync;
+    QEMUPutLEDEntry *led;
+    int ledstate;
     int key_delay_ms;
     QemuMutex mutex;
 
@@ -304,7 +306,6 @@  struct VncState
     size_t read_handler_expect;
     /* input */
     uint8_t modifiers_state[256];
-    QEMUPutLEDEntry *led;
 
     bool abort;
     QemuMutex output_mutex;