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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 25, 2014 at 11:23 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
> On Tue, 2014-11-25 at 14:19 +0100, Marc-André Lureau wrote:
>> Avoid deferencing session private data directly, and use accessors instead.
>
> typo: "deferencing"
>

ok

>> ---
>>  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);
>>      spice_session_set_migration_state(mig->session, SPICE_SESSION_MIGRATION_CONNECTING);
>
> Is there any situation where we want to set the migration state to
> _CONNECTING, but *NOT* set migration_copy to true? If not, can we simply
> let spice_session_set_migration_state() set this value for us, instead
> of requiring the caller to call both of these functions?

agree

>
>>
>>      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;
>>
>>      main_priv->migrate_data = &mig;
>>
>> diff --git a/gtk/channel-webdav.c b/gtk/channel-webdav.c
>> index 0529b59..dbb8730 100644
>> --- a/gtk/channel-webdav.c
>> +++ b/gtk/channel-webdav.c
>> @@ -336,13 +336,11 @@ static void magic_written(GObject *source_object,
>>      SpiceWebdavChannelPrivate *c = self->priv;
>>      gssize bytes_written;
>>      GError *err = NULL;
>> -    SpiceSession *session;
>>
>> -    session = spice_channel_get_session(SPICE_CHANNEL(self));
>>      bytes_written = g_output_stream_write_finish(G_OUTPUT_STREAM(source_object),
>>                                                   res, &err);
>>
>> -    if (err || bytes_written != sizeof(session->priv->webdav_magic))
>> +    if (err || bytes_written != WEBDAV_MAGIC_SIZE)
>>          goto error;
>>
>>      client_start_read(self, client);
>> @@ -396,7 +394,7 @@ static void client_connected(GObject *source_object,
>>      g_object_unref(output);
>>
>>      g_output_stream_write_async(g_io_stream_get_output_stream(G_IO_STREAM(conn)),
>> -                                session->priv->webdav_magic, sizeof(session->priv->webdav_magic),
>> +                                spice_session_get_webdav_magic(session), WEBDAV_MAGIC_SIZE,
>>                                  G_PRIORITY_DEFAULT, c->cancellable,
>>                                  magic_written, client);
>>      return;
>> @@ -654,7 +652,7 @@ static void new_connection(SoupSocket *sock,
>>      GSocketAddress *gaddr;
>>      GInetAddress *iaddr;
>>      guint port;
>> -    guint8 magic[16];
>> +    guint8 magic[WEBDAV_MAGIC_SIZE];
>>      gsize nread;
>>      gboolean success = FALSE;
>>      SoupSocketIOStatus status;
>> @@ -680,7 +678,7 @@ static void new_connection(SoupSocket *sock,
>>      g_object_set(new, "non-blocking", TRUE, NULL);
>>
>>      /* check we got the right magic */
>> -    if (memcmp(session->priv->webdav_magic, magic, sizeof(magic))) {
>> +    if (memcmp(spice_session_get_webdav_magic(session), magic, sizeof(magic))) {
>>          g_warn_if_reached();
>>          goto end;
>>      }
>> 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);
>
> I'd prefer the more gtk-ish convention of get_n_display_channels()
> rather than get_display_channels_count()

ok, I added a patch after, because there are many places in this code
with this name.

>> +void spice_session_set_migration_copy(SpiceSession *session, gboolean copy);
>> +void spice_session_set_main_channel(SpiceSession *session, SpiceChannel *channel);
>> +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);
>> +
>> +    return session->priv->usbredir;
>> +}
>> +
>> +G_GNUC_INTERNAL
>> +gboolean spice_session_get_smartcard_enabled(SpiceSession *session)
>> +{
>> +    g_return_val_if_fail(SPICE_IS_SESSION(session), FALSE);
>> +
>> +    return session->priv->smartcard;
>> +}
>> +
>> +G_GNUC_INTERNAL
>> +const guint8* spice_session_get_webdav_magic(SpiceSession *session)
>> +{
>> +    g_return_val_if_fail(SPICE_IS_SESSION(session), NULL);
>> +
>> +    return session->priv->webdav_magic;
>> +}
>> +
>> +G_GNUC_INTERNAL
>> +void spice_session_set_migration_copy(SpiceSession *session, gboolean copy)
>> +{
>> +    g_return_if_fail(SPICE_IS_SESSION(session));
>> +
>> +    session->priv->migration_copy = copy;
>> +}
>> +
>>  /**
>>   * spice_session_is_migration_copy:
>>   * @session: a Spice session
>> @@ -2364,3 +2404,25 @@ gboolean spice_session_is_migration_copy(SpiceSession *session)
>>
>>      return session->priv->migration_copy;
>>  }
>> +
>> +G_GNUC_INTERNAL
>> +void spice_session_set_main_channel(SpiceSession *session, SpiceChannel *channel)
>> +{
>> +    g_return_if_fail(SPICE_IS_SESSION(session));
>> +    g_return_if_fail(SPICE_IS_CHANNEL(channel));
>> +    g_return_if_fail(session->priv->cmain == NULL);
>> +
>> +    session->priv->cmain = channel;
>> +}
>> +
>> +G_GNUC_INTERNAL
>> +gboolean spice_session_set_migration_session(SpiceSession *session, SpiceSession *mig_session)
>> +{
>> +    g_return_val_if_fail(SPICE_IS_SESSION(session), FALSE);
>> +    g_return_val_if_fail(SPICE_IS_SESSION(mig_session), FALSE);
>> +    g_return_val_if_fail(session->priv->migration == NULL, FALSE);
>> +
>> +    session->priv->migration = mig_session;
>> +
>> +    return TRUE;
>> +}
>> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
>> index dbd3d1a..f80b657 100644
>> --- a/gtk/usb-device-manager.c
>> +++ b/gtk/usb-device-manager.c
>> @@ -1627,7 +1627,7 @@ spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager  *self,
>>      g_return_val_if_fail(device != NULL, FALSE);
>>      g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
>>
>> -    if (!priv->session->priv->usbredir) {
>> +    if (!spice_session_get_usbredir_enabled(priv->session)) {
>>          g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>>                              _("USB redirection is disabled"));
>>          return FALSE;
>
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]