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

["Daniel P. Berrange" <dan berrange com>]

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

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

> > Or redefine VNC_PADDING_LARGE to 16?

Doing this would be wrong because it would change other classes.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|