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

[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 |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 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..3f3fd15
--- /dev/null
+++ b/docs/vnc-ledstate-Pseudo-encoding.txt
@@ -0,0 +1,33 @@
+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.
+
+Example
+-------
+
+The example psuedo-encodings for LED state defined as following:
+
+======= ===============================================================
+Code    Description
+======= ===============================================================
+100     CapsLock is set
+010     NumLock is set
+001     ScrollLock is set
+110     CapsLock and NumLock are set
+111     CapsLock, NumLock and ScrollLock are set
+======= ===============================================================
-- 
1.7.7.6

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

[Anthony Liguori]

Lei Li <lilei@linux.vnet.ibm.com> writes:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  docs/vnc-ledstate-Pseudo-encoding.txt |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 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..3f3fd15
> --- /dev/null
> +++ b/docs/vnc-ledstate-Pseudo-encoding.txt
> @@ -0,0 +1,33 @@
> +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.
> +
> +Example
> +-------
> +
> +The example psuedo-encodings for LED state defined as following:
> +
> +======= ===============================================================
> +Code    Description
> +======= ===============================================================
> +100     CapsLock is set
> +010     NumLock is set
> +001     ScrollLock is set
> +110     CapsLock and NumLock are set
> +111     CapsLock, NumLock and ScrollLock are set
> +=======
> ===============================================================

You can just describe that each bit represents the Caps, Num, and Scroll
lock key respectively and that '1' indicates that the LED should be on
and '0' should be off.

You should also list the psuedo-encoding number somewhere in this document.

Regards,

Anthony Liguori

> -- 
> 1.7.7.6

[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