Re: [PATCH RFC v2] Add support for LED state extension to gvnc

[Lei Li <lilei linux vnet ibm com>]

On 05/07/2013 05:12 PM, Daniel P. Berrange wrote:
> On Tue, May 07, 2013 at 03:35:02PM +0800, Lei Li wrote:
> > On 05/07/2013 12:24 PM, Lei Li wrote:
> > > On 05/06/2013 02:47 AM, Daniel P. Berrange wrote:
> > > > On Sun, May 05, 2013 at 10:16:16PM +0800, Lei Li wrote:
> > > > > diff --git a/src/libgvnc_sym.version b/src/libgvnc_sym.version
> > > > > index 9adec25..20593ba 100644
> > > > > --- a/src/libgvnc_sym.version
> > > > > +++ b/src/libgvnc_sym.version
> > > > > @@ -87,6 +87,7 @@
> > > > >       vnc_connection_set_audio_format;
> > > > >       vnc_connection_get_audio_format;
> > > > >       vnc_connection_set_audio;
> > > > > +        vnc_connection_get_ledstate;
> > > > Indentation is off wrt other lines
> > > Oh, seems it needs a tab not blanks here..
> > >
> > > > > diff --git a/src/vncconnection.c b/src/vncconnection.c
> > > > > index 4b25a96..f86442c 100644
> > > > > --- a/src/vncconnection.c
> > > > > +++ b/src/vncconnection.c
> > > > > @@ -217,6 +217,7 @@ struct _VncConnectionPrivate
> > > > >       guint8 zrle_pi;
> > > > >       int zrle_pi_bits;
> > > > >   +    int ledstate;
> > > > What does this integer represent ?  I'm guessing you're somehow
> > > > using it as a bitfield ? If so, then you should be defining an
> > > > enum with some constants for each bit in the header file.
> > > Yes, you are right.
> > > OK, sure.
> > Sorry, my misunderstanding before. 'ledstate' is not a bitfield. In the implementation
> > over Qemu VNC server, it's just an integer.
> What does that integer represent though ? QEMU must have some sematics for it
> and you must define some semantics for it in the RFB protocol extension.  Just
> saying it is an integer is meaningless over QEMU side

The 'ledstate' represented in QEMU as:

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

#define QEMU_SCROLL_LOCK_LED (1 << 0)
#define QEMU_NUM_LOCK_LED    (1 << 1)
#define QEMU_CAPS_LOCK_LED   (1 << 2)


The semantics trying to define for RFB protocol has not been submitted,
since the previous proposal has some changes based on the comments from
QEMU. I planed to sent it out after this patch accepted.

Now there is just a document in QEMU that describes the semantics as following:

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

Some example encodings for it as following:

======= ===============================================================
Code    Description
======= ===============================================================
100     CapsLock is on, NumLock and ScrollLock are off
010     NumLock is on, CapsLock and ScrollLock are off
111     CapsLock, NumLock and ScrollLock are on
======= ===============================================================

Giving your concern above, do you think this description need a little
adjustment to make it more precise?//
 

> > > > No, you can't just insert new fields in the middle of a public
> > > > struct. You just broke ABI for apps linking to this.
> > > >
> > > > You need to add new fields at the end, and remove the corresponding
> > > > amount of padding
> > > Sorry, do you mean like this?
> > >
> > > gpointer _vnc_reserved[VNC_PADDING_LARGE - 4];
> Like this

OK, thanks!

> > > Or redefine VNC_PADDING_LARGE to 16?
> Doing this would be wrong because it would change other classes.
>
>
> Daniel


--
Lei