[Qemu-devel] [PATCH 0/3 v3] Support for LED state extension to Qemu VNC server

[Qemu-devel] [PATCH 0/3 v3] Support for LED state extension to Qemu VNC server

[Lei Li]

Hi guys,

This patch series tries to add support for LED state
extension to Qemu VNC server. The proposal has been sent
few days ago as link below:

http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg01421.html

The previous version as link:

http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03988.html

Pleae let me know if there is anything else need be improved.

Thanks!


Changes since v2:
  - Address the comments from Anthony includes:
    - Just send 1 instead of the actual width and height.
    - Improve the document by adding the Pseudo-encoding number
      and simplify the description of the LED state encoding.

Changes since v1:
  - Address the comments from Anthony includes:
    - Use Pseudo-encoding for led state;
    - Get rid of send_ext_leds_state_ack;
    - Add document for the led state Pseudo-encoding.

[Qemu-devel] [PATCH 1/3] vnc: Add SCROLL lock key to kbd_leds

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 ui/vnc.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 8ee66b7..f574962 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1525,10 +1525,11 @@ static void press_key(VncState *vs, int keysym)
 static void kbd_leds(void *opaque, int ledstate)
 {
     VncState *vs = opaque;
-    int caps, num;
+    int caps, num, scr;
 
     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 (vs->modifiers_state[0x3a] != caps) {
         vs->modifiers_state[0x3a] = caps;
@@ -1536,6 +1537,9 @@ static void kbd_leds(void *opaque, int ledstate)
     if (vs->modifiers_state[0x45] != num) {
         vs->modifiers_state[0x45] = num;
     }
+    if (vs->modifiers_state[0x46] != scr) {
+        vs->modifiers_state[0x46] = scr;
+    }
 }
 
 static void do_key_event(VncState *vs, int down, int keycode, int sym)
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/3] vnc: Support for LED state extension

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 ui/vnc.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 ui/vnc.h |    4 +++-
 2 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index f574962..44189d7 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1522,6 +1522,42 @@ static void press_key(VncState *vs, int keysym)
     kbd_put_keycode(keycode | SCANCODE_UP);
 }
 
+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_unlock_output(vs);
+    vnc_flush(vs);
+}
+
 static void kbd_leds(void *opaque, int ledstate)
 {
     VncState *vs = opaque;
@@ -1540,6 +1576,11 @@ static void kbd_leds(void *opaque, int ledstate)
     if (vs->modifiers_state[0x46] != scr) {
         vs->modifiers_state[0x46] = scr;
     }
+
+    /* Sending the current led state message to the client */
+    if (ledstate != current_led_state(vs)) {
+        vnc_led_state_change(vs);
+    }
 }
 
 static void do_key_event(VncState *vs, int down, int keycode, int sym)
@@ -1893,6 +1934,9 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
         case VNC_ENCODING_WMVi:
             vs->features |= VNC_FEATURE_WMVI_MASK;
             break;
+        case VNC_ENCODING_LED_STATE:
+            vs->features |= VNC_FEATURE_LED_STATE_MASK;
+            break;
         case VNC_ENCODING_COMPRESSLEVEL0 ... VNC_ENCODING_COMPRESSLEVEL0 + 9:
             vs->tight.compression = (enc & 0x0F);
             break;
@@ -1908,6 +1952,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
     }
     vnc_desktop_resize(vs);
     check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
+    vnc_led_state_change(vs);
 }
 
 static void set_pixel_conversion(VncState *vs)
diff --git a/ui/vnc.h b/ui/vnc.h
index ad1dec2..e873377 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -384,6 +384,7 @@ enum {
 #define VNC_ENCODING_EXT_KEY_EVENT        0XFFFFFEFE /* -258 */
 #define VNC_ENCODING_AUDIO                0XFFFFFEFD /* -259 */
 #define VNC_ENCODING_TIGHT_PNG            0xFFFFFEFC /* -260 */
+#define VNC_ENCODING_LED_STATE            0XFFFFFEFB /* -261 */
 #define VNC_ENCODING_WMVi                 0x574D5669
 
 /*****************************************************************************
@@ -422,6 +423,7 @@ enum {
 #define VNC_FEATURE_TIGHT_PNG                8
 #define VNC_FEATURE_ZRLE                     9
 #define VNC_FEATURE_ZYWRLE                  10
+#define VNC_FEATURE_LED_STATE               11
 
 #define VNC_FEATURE_RESIZE_MASK              (1 << VNC_FEATURE_RESIZE)
 #define VNC_FEATURE_HEXTILE_MASK             (1 << VNC_FEATURE_HEXTILE)
@@ -434,7 +436,7 @@ enum {
 #define VNC_FEATURE_TIGHT_PNG_MASK           (1 << VNC_FEATURE_TIGHT_PNG)
 #define VNC_FEATURE_ZRLE_MASK                (1 << VNC_FEATURE_ZRLE)
 #define VNC_FEATURE_ZYWRLE_MASK              (1 << VNC_FEATURE_ZYWRLE)
-
+#define VNC_FEATURE_LED_STATE_MASK           (1 << VNC_FEATURE_LED_STATE)
 
 /* Client -> Server message IDs */
 #define VNC_MSG_CLIENT_SET_PIXEL_FORMAT           0
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/3] doc: document the Pseudo-encoding of LED state

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 docs/vnc-ledstate-Pseudo-encoding.txt |   40 +++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)
 create mode 100644 docs/vnc-ledstate-Pseudo-encoding.txt

diff --git a/docs/vnc-ledstate-Pseudo-encoding.txt b/docs/vnc-ledstate-Pseudo-encoding.txt
new file mode 100644
index 0000000..0c312cf
--- /dev/null
+++ b/docs/vnc-ledstate-Pseudo-encoding.txt
@@ -0,0 +1,40 @@
+VNC LED state Pseudo-encoding
+=============================
+
+Introduction
+------------
+
+This document describes the Pseudo-encoding of LED state for RFB which
+is the protocol used in VNC as reference link below:
+
+http://tigervnc.svn.sourceforge.net/viewvc/tigervnc/rfbproto/rfbproto.rst?content-type=text/plain
+
+When accessing a guest by console through VNC, there might be mismatch
+between the lock keys notification LED on the computer running the VNC
+client session and the current status of the lock keys on the guest
+machine.
+
+To solve this problem it attempts to add LED state Pseudo-encoding
+extension to VNC protocol to deal with setting LED state.
+
+Pseudo-encoding
+---------------
+
+This Pseudo-encoding requested by client declares to server that it supports
+LED state extensions to the protocol.
+
+The Pseudo-encoding number for LED state defined as:
+
+======= ===============================================================
+Number  Name
+======= ===============================================================
+-261    'LED state Pseudo-encoding'_
+======= ===============================================================
+
+LED state Pseudo-enconding
+--------------------------
+
+The LED state Pseudo-encoding describes the encoding of LED state which
+consists of 3 bits, each bit represents the Caps, Num, and Scroll lock
+key respectively. '1' indicates that the LED should be on and '0' should
+be off.
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 2/3] vnc: Support for LED state extension

[Gerd Hoffmann]

  Hi,

> +        case VNC_ENCODING_LED_STATE:
> +            vs->features |= VNC_FEATURE_LED_STATE_MASK;
> +            break;

I think it with the client supporting the led state extension it is
probably a good idea to turn off the lock state sync logic in qemu's vnc
server, i.e. add a "!(vs->features & VNC_FEATURE_LED_STATE_MASK)" check
next to the lock_key_sync checks in do_key_event().

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/3 v3] Support for LED state extension to Qemu VNC server

[Eric Blake]

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

On 04/24/2013 04:12 AM, Lei Li wrote:
> Hi guys,
> 
> This patch series tries to add support for LED state
> extension to Qemu VNC server. The proposal has been sent
> few days ago as link below:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg01421.html
> 
> The previous version as link:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03988.html
> 
> Pleae let me know if there is anything else need be improved.

Can you fix your setup to include a series diffstat in your cover
letter?  I know that 'git send-email --cover-letter --annotate' does
this; but I also know that's not the workflow everyone uses.  I just
updated http://wiki.qemu.org/Contribute/SubmitAPatch to mention that a
cover letter diffstat is a useful review aid (I'd much rather check a
0/n letter to see what files are touched in summary, rather than opening
n emails just to see if it touches any file I care about).  I guess that
means I'm asking for a bit of help in further updating the wiki to
mention how to have git reliably include a cover letter diffstat, since
asking for something without giving a recipe of how to accomplish it
isn't very nice.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/3] doc: document the Pseudo-encoding of LED state

[Eric Blake]

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

On 04/24/2013 04:12 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  docs/vnc-ledstate-Pseudo-encoding.txt |   40 +++++++++++++++++++++++++++++++++
>  1 files changed, 40 insertions(+), 0 deletions(-)
>  create mode 100644 docs/vnc-ledstate-Pseudo-encoding.txt
> 
> +
> +The Pseudo-encoding number for LED state defined as:
> +
> +======= ===============================================================
> +Number  Name
> +======= ===============================================================
> +-261    'LED state Pseudo-encoding'_

Is the trailing _ intentional?

> +======= ===============================================================
> +
> +LED state Pseudo-enconding

s/enconding/encoding/

> +--------------------------
> +
> +The LED state Pseudo-encoding describes the encoding of LED state which
> +consists of 3 bits, each bit represents the Caps, Num, and Scroll lock
> +key respectively. '1' indicates that the LED should be on and '0' should
> +be off.

Which bit is which?  Is bit 0 the Caps lock LED?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 0/3 v3] Support for LED state extension to Qemu VNC server

[Lei Li]

On 04/24/2013 10:54 PM, Eric Blake wrote:
> On 04/24/2013 04:12 AM, Lei Li wrote:
>> Hi guys,
>>
>> This patch series tries to add support for LED state
>> extension to Qemu VNC server. The proposal has been sent
>> few days ago as link below:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg01421.html
>>
>> The previous version as link:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03988.html
>>
>> Pleae let me know if there is anything else need be improved.
> Can you fix your setup to include a series diffstat in your cover
> letter?  I know that 'git send-email --cover-letter --annotate' does
> this; but I also know that's not the workflow everyone uses.  I just
> updated http://wiki.qemu.org/Contribute/SubmitAPatch to mention that a
> cover letter diffstat is a useful review aid (I'd much rather check a
> 0/n letter to see what files are touched in summary, rather than opening
> n emails just to see if it touches any file I care about).  I guess that
> means I'm asking for a bit of help in further updating the wiki to
> mention how to have git reliably include a cover letter diffstat, since
> asking for something without giving a recipe of how to accomplish it
> isn't very nice.

Hi Eric,

Sure, I will include the series diffstat.
Thanks for your good suggestion!

>


-- 
Lei

Re: [Qemu-devel] [PATCH 3/3] doc: document the Pseudo-encoding of LED state

[Lei Li]

On 04/24/2013 10:57 PM, Eric Blake wrote:
> On 04/24/2013 04:12 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   docs/vnc-ledstate-Pseudo-encoding.txt |   40 +++++++++++++++++++++++++++++++++
>>   1 files changed, 40 insertions(+), 0 deletions(-)
>>   create mode 100644 docs/vnc-ledstate-Pseudo-encoding.txt
>>
>> +
>> +The Pseudo-encoding number for LED state defined as:
>> +
>> +======= ===============================================================
>> +Number  Name
>> +======= ===============================================================
>> +-261    'LED state Pseudo-encoding'_
> Is the trailing _ intentional?

Yes, add this trailing '_' to indicate that this is a quoted context and you can
locate its section somewhere.
I saw this style used in RFB protocol, but it's not necessary. I'd get rid of it
if you don't like it. :)

>
>> +======= ===============================================================
>> +
>> +LED state Pseudo-enconding
> s/enconding/encoding/

Sure.

>> +--------------------------
>> +
>> +The LED state Pseudo-encoding describes the encoding of LED state which
>> +consists of 3 bits, each bit represents the Caps, Num, and Scroll lock
>> +key respectively. '1' indicates that the LED should be on and '0' should
>> +be off.
> Which bit is which?  Is bit 0 the Caps lock LED?

Caps lock key is represented as '100', and Scroll lock key is represented as '001'.
Sorry, I should describe it more clearly.
Or maybe I should include the example of the encodings defined?

>


-- 
Lei

Re: [Qemu-devel] [PATCH 3/3] doc: document the Pseudo-encoding of LED state

[Eric Blake]

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

On 04/24/2013 08:25 PM, Lei Li wrote:
> On 04/24/2013 10:57 PM, Eric Blake wrote:
>> On 04/24/2013 04:12 AM, Lei Li wrote:
>>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>>> ---
>>>   docs/vnc-ledstate-Pseudo-encoding.txt |   40
>>> +++++++++++++++++++++++++++++++++
>>>   1 files changed, 40 insertions(+), 0 deletions(-)
>>>   create mode 100644 docs/vnc-ledstate-Pseudo-encoding.txt
>>>
>>> +
>>> +The Pseudo-encoding number for LED state defined as:
>>> +
>>> +======= ===============================================================
>>> +Number  Name
>>> +======= ===============================================================
>>> +-261    'LED state Pseudo-encoding'_
>> Is the trailing _ intentional?
> 
> Yes, add this trailing '_' to indicate that this is a quoted context and
> you can
> locate its section somewhere.
> I saw this style used in RFB protocol, but it's not necessary. I'd get
> rid of it
> if you don't like it. :)

I didn't read the RFB protocol; if you are being consistent with a
parent document that you are extending, then by all means use the same
conventions.

>> Which bit is which?  Is bit 0 the Caps lock LED?
> 
> Caps lock key is represented as '100', and Scroll lock key is
> represented as '001'.
> Sorry, I should describe it more clearly.

Yep, especially since your choice of encoding was opposite to mine.  A
good protocol should be easy to implement.  I don't care which order the
bits go in, as long as everyone treats the same bit as caps lock based
on good docs.

> Or maybe I should include the example of the encodings defined?

In my opinion, adding an example will never hurt :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/3] vnc: Support for LED state extension

[Lei Li]

On 04/24/2013 07:44 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> +        case VNC_ENCODING_LED_STATE:
>> +            vs->features |= VNC_FEATURE_LED_STATE_MASK;
>> +            break;
> I think it with the client supporting the led state extension it is
> probably a good idea to turn off the lock state sync logic in qemu's vnc
> server, i.e. add a "!(vs->features & VNC_FEATURE_LED_STATE_MASK)" check
> next to the lock_key_sync checks in do_key_event().
>
> cheers,
>    Gerd

Hi Gerd,

Yes, I think it's a good idea.
Thanks!

>


-- 
Lei