diff mbox

VNC LED state buggy, interop issue

Message ID 7ba29954-e3ad-529c-9978-b10d2788f9e4@cendio.se
State New
Headers show

Commit Message

Pierre Ossman Dec. 13, 2016, 1:10 p.m. UTC
On 13/12/16 09:06, Gerd Hoffmann wrote:
>>   b) Remove the assumption from the code and the protocol.
>
> Patches are welcome.
>

I was just aiming for a consensus on the intended behaviour, rather than 
fix the bug right now. :)
But all right, attached patch is an attempt.

Btw, is there any client that implements this extension yet? I couldn't 
find anything.

Regards

Comments

Daniel P. Berrangé Dec. 13, 2016, 1:15 p.m. UTC | #1
On Tue, Dec 13, 2016 at 02:10:04PM +0100, Pierre Ossman wrote:
> On 13/12/16 09:06, Gerd Hoffmann wrote:
> > >   b) Remove the assumption from the code and the protocol.
> > 
> > Patches are welcome.
> > 
> 
> I was just aiming for a consensus on the intended behaviour, rather than fix
> the bug right now. :)
> But all right, attached patch is an attempt.
> 
> Btw, is there any client that implements this extension yet? I couldn't find
> anything.

gtk-vnc implements the LED state extension.

IIRC, there have been bug reports about it being mostly useless with
QEMU, since it only reflects LED changes initiated by the guest,
and not those initiated by virtual key presses sent by GTK-VNC.


Regards,
Daniel
Pierre Ossman Dec. 13, 2016, 1:37 p.m. UTC | #2
On 13/12/16 14:15, Daniel P. Berrange wrote:
>>
>> Btw, is there any client that implements this extension yet? I couldn't find
>> anything.
>
> gtk-vnc implements the LED state extension.
>

Not enough to actually do anything useful AFAICT. That would be up to 
the application using gtk-vnc. And I couldn't see Vinagre using this 
feature. Any other frontend I should look at?

> IIRC, there have been bug reports about it being mostly useless with
> QEMU, since it only reflects LED changes initiated by the guest,
> and not those initiated by virtual key presses sent by GTK-VNC.

That sounds like this bug. It does respect the virtual keys sent, just 
that it doesn't inform the client properly. So my patch should help.

Regards
Pierre Ossman Dec. 13, 2016, 2:44 p.m. UTC | #3
On 13/12/16 14:15, Daniel P. Berrange wrote:
>
> gtk-vnc implements the LED state extension.
>

For testing, I've now pushed my work to get TigerVNC to support this:

https://github.com/CendioOssman/tigervnc/tree/ledstate

It's implemented for the client and Xvnc.

Regards
Gerd Hoffmann Dec. 14, 2016, 7:31 a.m. UTC | #4
On Di, 2016-12-13 at 14:10 +0100, Pierre Ossman wrote:
> On 13/12/16 09:06, Gerd Hoffmann wrote:
> >>   b) Remove the assumption from the code and the protocol.
> >
> > Patches are welcome.
> >
> 
> I was just aiming for a consensus on the intended behaviour, rather than 
> fix the bug right now. :)
> But all right, attached patch is an attempt.
> 
> Btw, is there any client that implements this extension yet? I couldn't 
> find anything.
> 
> Regards

Can you send that with git send-email instead of a attachment?
I can quote the patch for commenting then, also various tools expect
patches that way.

Patch looks good.  Commit message is too short.  You are moving the led
tracking from vncstate to vncdisplay, which makes sense, but this shpuld
be explained in the commit message too.

thanks,
  Gerd
diff mbox

Patch

From c27ac2026c84d7fd0da2c27a7ebe08f4359cc977 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <ossman@cendio.se>
Date: Tue, 13 Dec 2016 14:03:18 +0100
Subject: [PATCH] vnc: track LED state separately

Piggy-backing on the modifier state array made it difficult to send
out updates at the proper times.

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

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;
-- 
2.7.4