[Spice-devel] [PATCH spice-gtk 05/15] session: add and use internal accessors

[Christophe Fergeau]

On Tue, Nov 25, 2014 at 02:19:18PM +0100, Marc-André Lureau wrote:
> Avoid deferencing session private data directly, and use accessors instead.
> ---
>  gtk/channel-main.c       | 13 +++++-----
>  gtk/channel-webdav.c     | 10 ++++----
>  gtk/spice-channel.c      |  6 ++---
>  gtk/spice-session-priv.h |  9 +++++++
>  gtk/spice-session.c      | 62 ++++++++++++++++++++++++++++++++++++++++++++++++
>  gtk/usb-device-manager.c |  2 +-
>  6 files changed, 85 insertions(+), 17 deletions(-)
> 
> diff --git a/gtk/channel-main.c b/gtk/channel-main.c
> index 6d06ee3..0a820d7 100644
> --- a/gtk/channel-main.c
> +++ b/gtk/channel-main.c
> @@ -1298,7 +1298,7 @@ static gboolean timer_set_display(gpointer data)
>  
>      /* ensure we have an explicit monitor configuration at least for
>         number of display channels */
> -    for (i = 0; i < session->priv->display_channels_count; i++)
> +    for (i = 0; i < spice_session_get_display_channels_count(session); i++)
>          if (!c->display[i].enabled_set) {
>              SPICE_DEBUG("Not sending monitors config, missing monitors");
>              return FALSE;
> @@ -2073,7 +2073,7 @@ static gboolean migrate_connect(gpointer data)
>      g_return_val_if_fail(c != NULL, FALSE);
>      g_return_val_if_fail(mig->session != NULL, FALSE);
>  
> -    mig->session->priv->migration_copy = true;
> +    spice_session_set_migration_copy(mig->session, true);

I think this one could be set in spice_session_new_from_session (with
maybe some additional renaming of that method)

>      spice_session_set_migration_state(mig->session, SPICE_SESSION_MIGRATION_CONNECTING);
>  
>      if ((c->peer_hdr.major_version == 1) &&
> @@ -2133,7 +2133,8 @@ static gboolean migrate_connect(gpointer data)
>  
>      /* the migration process is in 2 steps, first the main channel and
>         then the rest of the channels */
> -    mig->session->priv->cmain = migrate_channel_connect(mig, SPICE_CHANNEL_MAIN, 0);
> +    spice_session_set_main_channel(mig->session,
> +                                   migrate_channel_connect(mig, SPICE_CHANNEL_MAIN, 0));
>  
>      return FALSE;
>  }
> @@ -2157,13 +2158,11 @@ static void main_migrate_connect(SpiceChannel *channel,
>  
>      CHANNEL_DEBUG(channel, "migrate connect");
>      session = spice_channel_get_session(channel);
> -    if (session->priv->migration != NULL)
> -        goto end;
> -
>      mig.session = spice_session_new_from_session(session);
>      if (mig.session == NULL)
>          goto end;
> -    session->priv->migration = mig.session;
> +    if (!spice_session_set_migration_session(session, mig.session))
> +        goto end;

Same here I guess (spice_session_set_migration_session could set
src_session->priv->migration = new_session when it's called).

> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 94f980f..154dacb 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -1987,7 +1987,7 @@ SpiceChannel *spice_channel_new(SpiceSession *s, int type, int id)
>          break;
>      case SPICE_CHANNEL_PLAYBACK:
>      case SPICE_CHANNEL_RECORD: {
> -        if (!s->priv->audio) {
> +        if (!spice_session_get_audio_enabled(s)) {
>              g_debug("audio channel is disabled, not creating it");
>              return NULL;
>          }
> @@ -1997,7 +1997,7 @@ SpiceChannel *spice_channel_new(SpiceSession *s, int type, int id)
>      }
>  #ifdef USE_SMARTCARD
>      case SPICE_CHANNEL_SMARTCARD: {
> -        if (!s->priv->smartcard) {
> +        if (!spice_session_get_smartcard_enabled(s)) {
>              g_debug("smartcard channel is disabled, not creating it");
>              return NULL;
>          }
> @@ -2007,7 +2007,7 @@ SpiceChannel *spice_channel_new(SpiceSession *s, int type, int id)
>  #endif
>  #ifdef USE_USBREDIR
>      case SPICE_CHANNEL_USBREDIR: {
> -        if (!s->priv->usbredir) {
> +        if (!spice_session_get_usbredir_enabled(s)) {
>              g_debug("usbredir channel is disabled, not creating it");
>              return NULL;
>          }
> diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
> index 55e2ed2..5a63536 100644
> --- a/gtk/spice-session-priv.h
> +++ b/gtk/spice-session-priv.h
> @@ -173,6 +173,15 @@ void spice_session_sync_playback_latency(SpiceSession *session);
>  const gchar* spice_session_get_shared_dir(SpiceSession *session);
>  void spice_session_set_shared_dir(SpiceSession *session, const gchar *dir);
>  gboolean spice_session_get_audio_enabled(SpiceSession *session);
> +gboolean spice_session_get_smartcard_enabled(SpiceSession *session);
> +gboolean spice_session_get_usbredir_enabled(SpiceSession *session);
> +
> +#define WEBDAV_MAGIC_SIZE 16
> +const guint8* spice_session_get_webdav_magic(SpiceSession *session);
> +guint spice_session_get_display_channels_count(SpiceSession *session);
> +void spice_session_set_migration_copy(SpiceSession *session, gboolean copy);
> +gboolean spice_session_set_migration_session(SpiceSession *session, SpiceSession *mig_session);
>  
>  G_END_DECLS
>  
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index 9d5cb13..5dd6f0f 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -2198,6 +2198,14 @@ void spice_session_set_caches_hints(SpiceSession *session,
>  }
>  
>  G_GNUC_INTERNAL
> +guint spice_session_get_display_channels_count(SpiceSession *session)
> +{
> +    g_return_val_if_fail(session != NULL, 0);
> +
> +    return session->priv->display_channels_count;
> +}
> +
> +G_GNUC_INTERNAL
>  void spice_session_set_uuid(SpiceSession *session, guint8 uuid[16])
>  {
>      g_return_if_fail(session != NULL);
> @@ -2346,6 +2354,38 @@ gboolean spice_session_get_audio_enabled(SpiceSession *session)
>      return session->priv->audio;
>  }
>  
> +G_GNUC_INTERNAL
> +gboolean spice_session_get_usbredir_enabled(SpiceSession *session)
> +{
> +    g_return_val_if_fail(SPICE_IS_SESSION(session), FALSE);

You used g_return_val_if_fail(session != NULL, 0); in patch 2 and in
get_display_channels_count above, and then the rest of the patch uses
SPICE_IS_SESSION(), it's probably better to have something consistent.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20141126/e03a17b1/attachment-0001.sig>