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

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

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

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

> 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


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