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

[Lei Li <lilei linux vnet ibm com>]

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.

Hi Daniel,

Sorry, my misunderstanding before. 'ledstate' is not a bitfield. In the implementation
over Qemu VNC server, it's just an integer.

> > > diff --git a/src/vncconnection.h b/src/vncconnection.h
> > > index cbfefd7..f2fc19b 100644
> > > --- a/src/vncconnection.h
> > > +++ b/src/vncconnection.h
> > > @@ -65,6 +65,7 @@ struct _VncConnectionClass
> > >       void (*vnc_framebuffer_update)(VncConnection *conn, guint16 x, 
> > > guint16 y, guint16 width, guint16 height);
> > >       void (*vnc_desktop_resize)(VncConnection *conn, guint16 width, 
> > > guint16 height);
> > >       void (*vnc_pixel_format_changed)(VncConnection *conn, 
> > > VncPixelFormat *format);
> > > +    void (*vnc_led_state)(VncConnection *conn);
> > >       void (*vnc_auth_failure)(VncConnection *conn, const char 
> > > *reason);
> > >       void (*vnc_auth_unsupported)(VncConnection *conn, unsigned int 
> > > authType);
> > >       void (*vnc_auth_credential)(VncConnection *conn, GValueArray 
> > > *creds);
> >
> > 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];
>
> Or redefine VNC_PADDING_LARGE to 16?
>
> > Daniel


--
Lei