[Qemu-devel] VNC LED state buggy, interop issue

[Qemu-devel] VNC LED state buggy, interop issue

[Pierre Ossman]

Hi,

I'm working on adding support for the LED state extension in TigerVNC. 
Unfortunately I discovered some bugs in the QEMU implementation so I 
need to discuss with you guys what the proper behaviour should be.

The basic problem is that the code right now assumes that XK_Caps_Lock 
and XK_Num_Lock will toggle their respective states. It is however not 
assumed that XK_Scroll_Lock toggles any state.

This assumption can of course be wrong. Simply run this in your guest to 
break things:

   xmodmap -e 'clear mod2'

Pressing NumLock will now toggle the LED and state on the client side 
(that's what is assumed), but not on the server side. However the client 
is never informed by the server that things are out of sync.

There are two ways to fix this:

  a) Send an update to the client when the assumption doesn't hold. This 
will most likely be difficult in your case since there is no definite 
point where you can assume a LED change event should have occurred.

  b) Remove the assumption from the code and the protocol.

My vote is for b). Assumptions and side effects are rarely a good idea. 
The downside though is that it will break compatibility with older 
versions of QEMU.

Regards
-- 
Pierre Ossman           Software Development
Cendio AB		http://cendio.com
Teknikringen 8		http://twitter.com/ThinLinc
583 30 Linköping	http://facebook.com/ThinLinc
Phone: +46-13-214600	http://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Re: [Qemu-devel] VNC LED state buggy, interop issue

[Pierre Ossman]

On 10/12/16 16:30, Pierre Ossman wrote:
>
> There are two ways to fix this:
>
>  a) Send an update to the client when the assumption doesn't hold. This
> will most likely be difficult in your case since there is no definite
> point where you can assume a LED change event should have occurred.
>
>  b) Remove the assumption from the code and the protocol.
>
> My vote is for b). Assumptions and side effects are rarely a good idea.
> The downside though is that it will break compatibility with older
> versions of QEMU.
>

Follow-up: Reading the initial proposal, it does indeed seem that b) was 
the intended behaviour:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg165162.html

Regards
-- 
Pierre Ossman           Software Development
Cendio AB               https://cendio.com
Teknikringen 8          https://twitter.com/ThinLinc
583 30 Linköping        https://facebook.com/ThinLinc
Phone: +46-13-214600    https://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Re: [Qemu-devel] VNC LED state buggy, interop issue

[Gerd Hoffmann]

  Hi,

> The basic problem is that the code right now assumes that XK_Caps_Lock 
> and XK_Num_Lock will toggle their respective states. It is however not 
> assumed that XK_Scroll_Lock toggles any state.

The whole thing is more intended to make sure guest and host have the
same idea about numlock and capslock state, so input works propery, not
so much about making the leds blink properly.

And, yes, it assumes you don't remap capslock + numlock keys to
something else (in the guest).  vnc has an option (lock-key-sync) to
turn off this logic in case it causes problems because the assumtion
doesn't hold for a specific guest.

>   b) Remove the assumption from the code and the protocol.

Patches are welcome.

cheers,
  Gerd

Re: [Qemu-devel] VNC LED state buggy, interop issue

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

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
-- 
Pierre Ossman           Software Development
Cendio AB		http://cendio.com
Teknikringen 8		http://twitter.com/ThinLinc
583 30 Linköping	http://facebook.com/ThinLinc
Phone: +46-13-214600	http://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #2: 0001-vnc-track-LED-state-separately.patch --]
[-- Type: text/x-patch, Size: 4567 bytes --]

>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

Re: [Qemu-devel] VNC LED state buggy, interop issue

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] VNC LED state buggy, interop issue

[Pierre Ossman]

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           Software Development
Cendio AB		http://cendio.com
Teknikringen 8		http://twitter.com/ThinLinc
583 30 Linköping	http://facebook.com/ThinLinc
Phone: +46-13-214600	http://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Re: [Qemu-devel] VNC LED state buggy, interop issue

[Pierre Ossman]

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
-- 
Pierre Ossman           Software Development
Cendio AB		http://cendio.com
Teknikringen 8		http://twitter.com/ThinLinc
583 30 Linköping	http://facebook.com/ThinLinc
Phone: +46-13-214600	http://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Re: [Qemu-devel] VNC LED state buggy, interop issue

[Gerd Hoffmann]

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

[Qemu-devel] VNC LED state buggy, interop issue

[Pierre Ossman]

Hi,

I'm working on adding support for the LED state extension in TigerVNC. 
Unfortunately I discovered some bugs in the QEMU implementation so I 
need to discuss with you guys what the proper behaviour should be.

The basic problem is that the code right now assumes that XK_Caps_Lock 
and XK_Num_Lock will toggle their respective states. It is however not 
assumed that XK_Scroll_Lock toggles any state.

This assumption can of course be wrong. Simply run this in your guest to 
break things:

   xmodmap -e 'clear mod2'

Pressing NumLock will now toggle the LED and state on the client side 
(that's what is assumed), but not on the server side. However the client 
is never informed by the server that things are out of sync.

There are two ways to fix this:

  a) Send an update to the client when the assumption doesn't hold. This 
will most likely be difficult in your case since there is no definite 
point where you can assume a LED change event should have occurred.

  b) Remove the assumption from the code and the protocol.

My vote is for b). Assumptions and side effects are rarely a good idea. 
The downside though is that it will break compatibility with older 
versions of QEMU.

Regards
-- 
Pierre Ossman           Software Development
Cendio AB		http://cendio.com
Teknikringen 8		http://twitter.com/ThinLinc
583 30 Linköping	http://facebook.com/ThinLinc
Phone: +46-13-214600	http://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?