[Spice-devel] [PATCH spice-gtk 3/3] RFC: channel: use class private handlers field

[Fabiano Fidencio]

----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau at redhat.com>
> To: spice-devel at lists.freedesktop.org
> Sent: Friday, June 5, 2015 6:29:44 PM
> Subject: [Spice-devel] [PATCH spice-gtk 3/3] RFC: channel: use class private	handlers field
> 
> Since spice-gtk requires glib 2.28, we can now fix a small FIXME.
> 
> Since G_TYPE_CLASS_GET_PRIVATE is a bit expensive, it's still worth to
> cache it in klass->priv. However, there is no good place I can think of
> to put this. (channel_class_init() is called only once, and not per
> each subclass)
> ---
>  src/channel-base.c       | 16 +++++++++-------
>  src/spice-channel-priv.h |  5 +++++
>  src/spice-channel.c      |  7 ++++---
>  src/spice-channel.h      |  4 +++-
>  4 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/src/channel-base.c b/src/channel-base.c
> index 77d339c..13e4ced 100644
> --- a/src/channel-base.c
> +++ b/src/channel-base.c
> @@ -197,7 +197,7 @@ end:
>  }
>  
>  
> -static void set_handlers(SpiceChannelClass *klass,
> +static void set_handlers(SpiceChannelClassPrivate *klass,
>                           const spice_msg_handler* handlers, const int n)
>  {
>      int i;
> @@ -209,7 +209,7 @@ static void set_handlers(SpiceChannelClass *klass,
>      }
>  }
>  
> -static void spice_channel_add_base_handlers(SpiceChannelClass *klass)
> +static void spice_channel_add_base_handlers(SpiceChannelClassPrivate *klass)
>  {
>      static const spice_msg_handler handlers[] = {
>          [ SPICE_MSG_SET_ACK ]                  =
>          spice_channel_handle_set_ack,
> @@ -227,12 +227,14 @@ G_GNUC_INTERNAL
>  void spice_channel_set_handlers(SpiceChannelClass *klass,
>                                  const spice_msg_handler* handlers, const int
>                                  n)
>  {
> -    /* FIXME: use class private (requires glib 2.24) */
> -    g_return_if_fail(klass->handlers == NULL);
> -    klass->handlers = g_array_sized_new(FALSE, TRUE,
> sizeof(spice_msg_handler), n);
> +    klass->priv =
> +        G_TYPE_CLASS_GET_PRIVATE (klass, spice_channel_get_type (),
> SpiceChannelClassPrivate);
>  
> -    spice_channel_add_base_handlers(klass);
> -    set_handlers(klass, handlers, n);
> +    g_return_if_fail(klass->priv->handlers == NULL);
> +    klass->priv->handlers = g_array_sized_new(FALSE, TRUE,
> sizeof(spice_msg_handler), n);
> +
> +    spice_channel_add_base_handlers(klass->priv);
> +    set_handlers(klass->priv, handlers, n);
>  }
>  
>  static void
> diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
> index d70cf86..436a521 100644
> --- a/src/spice-channel-priv.h
> +++ b/src/spice-channel-priv.h
> @@ -74,6 +74,11 @@ enum spice_channel_state {
>      SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE,
>  };
>  
> +struct _SpiceChannelClassPrivate
> +{
> +    GArray *handlers;
> +};
> +
>  struct _SpiceChannelPrivate {
>      /* swapped on migration */
>      SSL_CTX                     *ctx;
> diff --git a/src/spice-channel.c b/src/spice-channel.c
> index 4e7d8b7..0585a01 100644
> --- a/src/spice-channel.c
> +++ b/src/spice-channel.c
> @@ -76,7 +76,8 @@ static gboolean channel_connect(SpiceChannel *channel,
> gboolean tls);
>  #define SPICE_CHANNEL_GET_PRIVATE(obj)                                  \
>      (G_TYPE_INSTANCE_GET_PRIVATE ((obj), SPICE_TYPE_CHANNEL,
>      SpiceChannelPrivate))
>  
> -G_DEFINE_TYPE(SpiceChannel, spice_channel, G_TYPE_OBJECT);
> +G_DEFINE_TYPE_WITH_CODE (SpiceChannel, spice_channel, G_TYPE_OBJECT,
> +                         g_type_add_class_private (g_define_type_id, sizeof
> (SpiceChannelClassPrivate)));
>  
>  /* Properties */
>  enum {
> @@ -2848,11 +2849,11 @@ static void spice_channel_handle_msg(SpiceChannel
> *channel, SpiceMsgIn *msg)
>      int type = spice_msg_in_type(msg);
>      spice_msg_handler handler;
>  
> -    g_return_if_fail(type < klass->handlers->len);
> +    g_return_if_fail(type < klass->priv->handlers->len);
>      if (type > SPICE_MSG_BASE_LAST && channel->priv->disable_channel_msg)
>          return;
>  
> -    handler = g_array_index(klass->handlers, spice_msg_handler, type);
> +    handler = g_array_index(klass->priv->handlers, spice_msg_handler, type);
>      g_return_if_fail(handler != NULL);
>      handler(channel, msg);
>  }
> diff --git a/src/spice-channel.h b/src/spice-channel.h
> index 7f132f6..e0ce443 100644
> --- a/src/spice-channel.h
> +++ b/src/spice-channel.h
> @@ -68,6 +68,8 @@ struct _SpiceChannel
>      /* Do not add fields to this struct */
>  };
>  
> +typedef struct _SpiceChannelClassPrivate SpiceChannelClassPrivate;
> +
>  struct _SpiceChannelClass
>  {
>      GObjectClass parent_class;
> @@ -94,7 +96,7 @@ struct _SpiceChannelClass
>      /* virtual methods, coroutine context */
>      void (*channel_send_migration_handshake)(SpiceChannel *channel);
>  
> -    GArray                      *handlers;
> +    SpiceChannelClassPrivate *priv;
>      /*
>       * If adding fields to this struct, remove corresponding
>       * amount of padding to avoid changing overall struct size
> --
> 2.4.2
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 

Looks good to me, but I didn't test the patch.
Consider it as an ACK if you have it tested.