On Wed, Nov 26, 2014 at 11:54 AM, Christophe Fergeau
<cfergeau@xxxxxxxxxx> wrote:
> 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)
Hmm, I prefer to keep that function not specialized for migration, it
could become public (sort of a copy constructor)
>> 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).
same reason
>
>> 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
it seems to use consistently FALSE/0 where appropriate.
> get_display_channels_count above, and then the rest of the patch uses
> SPICE_IS_SESSION(), it's probably better to have something consistent.
I don't think that matters much, SPICE_IS_SESSION is better, but they
are internal checks. I'll replace by IS_SESSION in patch 2.
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel