[Spice-devel] [PATCH spice-gtk] Running out of reserved space, break ABI
Christophe Fergeau
cfergeau at redhat.com
Mon May 21 02:35:50 PDT 2012
Hey,
On Thu, May 17, 2012 at 04:20:12PM +0200, Marc-André Lureau wrote:
> The change abc56811de978ad336a651924a21b920cfe677f0 actually added
> a field in a public struct while changing overall struct size, the
> fields were also reorder, all of this breaks ABI.
>
> However, we are running out of free space in SpiceChannelClass
> struct. And since the SPICE_RESERVED_PADDING was 44 bytes, that is
> quite limited on 64bits (only 5 pointers fit).
>
> I propose we break ABI during this cycle. This means that programs
> using spice-gtk will need to be recompiled to use the new library.
> (the old library should be parallel installable though). This let us:
Yes, makes sense to me
> - use a better SPICE_RESERVED_PADDING based on pointer size, since
> it is what is usually added for virtual methods
> - reset the amount taken from the padding in the various struct
> - reorder fields a little
> - add some missing "priv" pointers
Splitting the priv addition would make this diff more readable ;)
> - whatever I am missing that we can still change before next release
>
> Please comment if I am missing something, or correct me
> ---
> gtk/Makefile.am | 4 ++--
> gtk/channel-playback.h | 6 +-----
> gtk/channel-record.h | 6 +-----
> gtk/spice-audio.c | 10 +++++-----
> gtk/spice-audio.h | 5 +++--
> gtk/spice-channel.h | 16 ++++++++--------
> gtk/spice-util.h | 2 +-
> gtk/usb-device-manager.h | 2 +-
> 8 files changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/gtk/Makefile.am b/gtk/Makefile.am
> index 7b29e61..0327d65 100644
> --- a/gtk/Makefile.am
> +++ b/gtk/Makefile.am
> @@ -91,7 +91,7 @@ AM_CPPFLAGS = \
>
> # http://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
> SPICE_GTK_LDFLAGS_COMMON = \
> - -version-info 3:0:2 \
> + -version-info 4:0:0 \
> -no-undefined \
> $(VERSION_LDFLAGS) \
> $(NULL)
> @@ -158,7 +158,7 @@ nodist_libspice_client_gtkinclude_HEADERS = \
> $(NULL)
>
> libspice_client_glib_2_0_la_LDFLAGS = \
> - -version-info 7:0:6 \
> + -version-info 8:0:0 \
> -no-undefined \
> $(VERSION_LDFLAGS) \
> $(NULL)
> diff --git a/gtk/channel-playback.h b/gtk/channel-playback.h
> index 6341cac..9cf68cf 100644
> --- a/gtk/channel-playback.h
> +++ b/gtk/channel-playback.h
> @@ -65,11 +65,7 @@ struct _SpicePlaybackChannelClass {
> void (*playback_stop)(SpicePlaybackChannel *channel);
>
> /*< private >*/
> - /*
> - * If adding fields to this struct, remove corresponding
> - * amount of padding to avoid changing overall struct size
> - */
> - gchar _spice_reserved[SPICE_RESERVED_PADDING];
> + /* Do not add fields to this struct */
> };
The reason for doing this change is not described in the changelog I think.
Are they private structs ?
Christophe
>
> GType spice_playback_channel_get_type(void);
> diff --git a/gtk/channel-record.h b/gtk/channel-record.h
> index 73bdb76..20a9ad3 100644
> --- a/gtk/channel-record.h
> +++ b/gtk/channel-record.h
> @@ -65,11 +65,7 @@ struct _SpiceRecordChannelClass {
> void (*record_stop)(SpiceRecordChannel *channel);
>
> /*< private >*/
> - /*
> - * If adding fields to this struct, remove corresponding
> - * amount of padding to avoid changing overall struct size
> - */
> - gchar _spice_reserved[SPICE_RESERVED_PADDING];
> + /* Do not add fields to this struct */
> };
>
> GType spice_record_channel_get_type(void);
> diff --git a/gtk/spice-audio.c b/gtk/spice-audio.c
> index 30c2920..6cf8f01 100644
> --- a/gtk/spice-audio.c
> +++ b/gtk/spice-audio.c
> @@ -67,7 +67,7 @@ enum {
> static void spice_audio_finalize(GObject *gobject)
> {
> SpiceAudio *self = SPICE_AUDIO(gobject);
> - SpiceAudioPrivate *priv = SPICE_AUDIO_GET_PRIVATE(self);
> + SpiceAudioPrivate *priv = self->priv;
>
> if (priv->main_context) {
> g_main_context_unref(priv->main_context);
> @@ -84,7 +84,7 @@ static void spice_audio_get_property(GObject *gobject,
> GParamSpec *pspec)
> {
> SpiceAudio *self = SPICE_AUDIO(gobject);
> - SpiceAudioPrivate *priv = SPICE_AUDIO_GET_PRIVATE(self);
> + SpiceAudioPrivate *priv = self->priv;
>
> switch (prop_id) {
> case PROP_SESSION:
> @@ -105,7 +105,7 @@ static void spice_audio_set_property(GObject *gobject,
> GParamSpec *pspec)
> {
> SpiceAudio *self = SPICE_AUDIO(gobject);
> - SpiceAudioPrivate *priv = SPICE_AUDIO_GET_PRIVATE(self);
> + SpiceAudioPrivate *priv = self->priv;
>
> switch (prop_id) {
> case PROP_SESSION:
> @@ -152,9 +152,9 @@ static void spice_audio_class_init(SpiceAudioClass *klass)
> g_type_class_add_private(klass, sizeof(SpiceAudioPrivate));
> }
>
> -static void spice_audio_init(SpiceAudio *self G_GNUC_UNUSED)
> +static void spice_audio_init(SpiceAudio *self)
> {
> - /* FIXME: self->priv = SPICE_AUDIO_GET_PRIVATE(audio) when ABI break */
> + self->priv = SPICE_AUDIO_GET_PRIVATE(self);
> }
>
> static void connect_channel(SpiceAudio *self, SpiceChannel *channel)
> diff --git a/gtk/spice-audio.h b/gtk/spice-audio.h
> index 732fea1..ebc4946 100644
> --- a/gtk/spice-audio.h
> +++ b/gtk/spice-audio.h
> @@ -52,7 +52,8 @@ typedef struct _SpiceAudioPrivate SpiceAudioPrivate;
> */
> struct _SpiceAudio {
> GObject parent;
> - /* FIXME: break ABI!! SpiceAudioPrivate *priv; */
> +
> + SpiceAudioPrivate *priv;
> };
>
> /**
> @@ -67,7 +68,7 @@ struct _SpiceAudioClass {
> /*< private >*/
> gboolean (*connect_channel)(SpiceAudio *audio, SpiceChannel *channel);
>
> - gchar _spice_reserved[SPICE_RESERVED_PADDING - sizeof(void*)];
> + gchar _spice_reserved[SPICE_RESERVED_PADDING];
> };
>
> GType spice_audio_get_type(void);
> diff --git a/gtk/spice-channel.h b/gtk/spice-channel.h
> index f722c99..982b73b 100644
> --- a/gtk/spice-channel.h
> +++ b/gtk/spice-channel.h
> @@ -72,29 +72,29 @@ struct _SpiceChannelClass
> {
> GObjectClass parent_class;
>
> + /*< public >*/
> + /* signals, main context */
> + void (*channel_event)(SpiceChannel *channel, SpiceChannelEvent event);
> + void (*open_fd)(SpiceChannel *channel, int with_tls);
> +
> /*< private >*/
> /* virtual methods, coroutine context */
> void (*handle_msg)(SpiceChannel *channel, SpiceMsgIn *msg);
> void (*channel_up)(SpiceChannel *channel);
> void (*iterate_write)(SpiceChannel *channel);
> void (*iterate_read)(SpiceChannel *channel);
> - void (*channel_reset_capabilities)(SpiceChannel *channel);
> -
> - /*< public >*/
> - /* signals, main context */
> - void (*channel_event)(SpiceChannel *channel, SpiceChannelEvent event);
> - void (*open_fd)(SpiceChannel *channel, int with_tls);
>
> /*< private >*/
> /* virtual method, any context */
> void (*channel_disconnect)(SpiceChannel *channel);
> -
> void (*channel_reset)(SpiceChannel *channel, gboolean migrating);
> + void (*channel_reset_capabilities)(SpiceChannel *channel);
> +
> /*
> * If adding fields to this struct, remove corresponding
> * amount of padding to avoid changing overall struct size
> */
> - gchar _spice_reserved[SPICE_RESERVED_PADDING - 5 * sizeof(void*)];
> + gchar _spice_reserved[SPICE_RESERVED_PADDING];
> };
>
> GType spice_channel_get_type(void);
> diff --git a/gtk/spice-util.h b/gtk/spice-util.h
> index 5bd24d3..7a617f4 100644
> --- a/gtk/spice-util.h
> +++ b/gtk/spice-util.h
> @@ -37,7 +37,7 @@ gulong spice_g_signal_connect_object(gpointer instance,
> g_debug(G_STRLOC " " fmt, ## __VA_ARGS__); \
> } while (0)
>
> -#define SPICE_RESERVED_PADDING 44
> +#define SPICE_RESERVED_PADDING (10 * sizeof(void*))
>
> #ifndef SPICE_NO_DEPRECATED
> #define SPICE_DEPRECATED_FOR(f) \
> diff --git a/gtk/usb-device-manager.h b/gtk/usb-device-manager.h
> index ba917da..df138ee 100644
> --- a/gtk/usb-device-manager.h
> +++ b/gtk/usb-device-manager.h
> @@ -82,7 +82,7 @@ struct _SpiceUsbDeviceManagerClass
> * If adding fields to this struct, remove corresponding
> * amount of padding to avoid changing overall struct size
> */
> - gchar _spice_reserved[SPICE_RESERVED_PADDING - sizeof(void*)];
> + gchar _spice_reserved[SPICE_RESERVED_PADDING];
> };
>
> GType spice_usb_device_get_type(void);
> --
> 1.7.10.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20120521/3f50595f/attachment-0001.pgp>
More information about the Spice-devel
mailing list