[Spice-devel] [PATCH spice-server v3 2/3] Improve MainChannel encapsulation

[Frediano Ziglio]

> 
> From: Jonathon Jongsma <jjongsma at redhat.com>
> 
> Encapsulate MainChannel a bit better in preparation for proting to
> GObject.
> ---
>  server/main-channel-client.c | 24 +++++++++++-------------
>  server/main-channel.c        | 15 +++++++++++++++
>  server/main-channel.h        |  2 ++
>  3 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/server/main-channel-client.c b/server/main-channel-client.c
> index 0913028..28d2839 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -439,11 +439,7 @@ void
> main_channel_client_handle_migrate_connected(MainChannelClient *mcc,
>  
>          mcc->priv->mig_wait_connect = FALSE;
>          mcc->priv->mig_connect_ok = success;
> -        spice_assert(main_channel->num_clients_mig_wait);
> -        spice_assert(!seamless || main_channel->num_clients_mig_wait == 1);
> -        if (!--main_channel->num_clients_mig_wait) {
> -            reds_on_main_migrate_connected(channel->reds, seamless &&
> success);
> -        }
> +        main_channel_on_migrate_connected(main_channel, seamless &&
> success);
>      } else {
>          if (success) {
>              spice_printerr("client %p MIGRATE_CANCEL", client);
> @@ -876,7 +872,7 @@ static void main_channel_marshall_notify(RedChannelClient
> *rcc,
>  static void main_channel_fill_migrate_dst_info(MainChannel *main_channel,
>                                                 SpiceMigrationDstInfo
>                                                 *dst_info)
>  {
> -    RedsMigSpice *mig_dst = &main_channel->mig_target;
> +    const RedsMigSpice *mig_dst =
> main_channel_peek_migration_target(main_channel);
>      dst_info->port = mig_dst->port;
>      dst_info->sport = mig_dst->sport;
>      dst_info->host_size = strlen(mig_dst->host) + 1;
> @@ -935,17 +931,19 @@ static void
> main_channel_marshall_migrate_switch(SpiceMarshaller *m, RedChannelC
>      RedChannel *channel = red_channel_client_get_channel(rcc);
>      SpiceMsgMainMigrationSwitchHost migrate;
>      MainChannel *main_ch;
> +    const RedsMigSpice *mig_target;
>  
>      spice_printerr("");
>      red_channel_client_init_send_data(rcc,
>      SPICE_MSG_MAIN_MIGRATE_SWITCH_HOST, item);
>      main_ch = SPICE_CONTAINEROF(channel, MainChannel, base);
> -    migrate.port = main_ch->mig_target.port;
> -    migrate.sport = main_ch->mig_target.sport;
> -    migrate.host_size = strlen(main_ch->mig_target.host) + 1;
> -    migrate.host_data = (uint8_t *)main_ch->mig_target.host;
> -    if (main_ch->mig_target.cert_subject) {
> -        migrate.cert_subject_size = strlen(main_ch->mig_target.cert_subject)
> + 1;
> -        migrate.cert_subject_data = (uint8_t
> *)main_ch->mig_target.cert_subject;
> +    mig_target = main_channel_peek_migration_target(main_ch);
> +    migrate.port = mig_target->port;
> +    migrate.sport = mig_target->sport;
> +    migrate.host_size = strlen(mig_target->host) + 1;
> +    migrate.host_data = (uint8_t *)mig_target->host;
> +    if (mig_target->cert_subject) {
> +        migrate.cert_subject_size = strlen(mig_target->cert_subject) + 1;
> +        migrate.cert_subject_data = (uint8_t *)mig_target->cert_subject;
>      } else {
>          migrate.cert_subject_size = 0;
>          migrate.cert_subject_data = NULL;
> diff --git a/server/main-channel.c b/server/main-channel.c
> index a1b8e31..2d2783d 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -419,3 +419,18 @@ int main_channel_migrate_src_complete(MainChannel
> *main_chan, int success)
>     }
>     return semi_seamless_count;
>  }
> +
> +void main_channel_on_migrate_connected(MainChannel *main_channel, gboolean
> seamless)
> +{
> +        g_return_if_fail(main_channel->num_clients_mig_wait);
> +        g_warn_if_fail(!seamless || main_channel->num_clients_mig_wait ==
> 1);

I just realized that beside spice_assert replacement this test is different.
Not seamless is the old "seamless && success" condition so this is equivalent
to 

g_warn_if_fail(!(seamless && success) || main_channel->num_clients_mig_wait == 1);

which is

g_warn_if_fail(!seamless || !success || main_channel->num_clients_mig_wait == 1);

we should probably pass seamless and success.

Perhaps would be more easier to define a new enum like

enum {
    MIG_CONNECT_STANDARD,
    MIG_CONNECT_SEAMLESS,
    MIG_CONNECT_ERROR
};

that group success and seamless.

Still convinced that changing spice_assert has nothing to do which
this patch rationale.

> +        if (!--main_channel->num_clients_mig_wait) {
> +
> reds_on_main_migrate_connected(red_channel_get_server(RED_CHANNEL(main_channel)),
> +                                           seamless);
> +        }
> +}
> +
> +const RedsMigSpice* main_channel_peek_migration_target(MainChannel
> *main_chan)
> +{
> +    return &main_chan->mig_target;
> +}
> diff --git a/server/main-channel.h b/server/main-channel.h
> index e0858d0..529e7be 100644
> --- a/server/main-channel.h
> +++ b/server/main-channel.h
> @@ -78,7 +78,9 @@ void main_channel_migrate_switch(MainChannel *main_chan,
> RedsMigSpice *mig_targe
>  int main_channel_migrate_connect(MainChannel *main_channel, RedsMigSpice
>  *mig_target,
>                                   int try_seamless);
>  void main_channel_migrate_cancel_wait(MainChannel *main_chan);
> +const RedsMigSpice* main_channel_peek_migration_target(MainChannel
> *main_chan);
>  /* returns the number of clients for which SPICE_MSG_MAIN_MIGRATE_END was
>  sent*/
>  int main_channel_migrate_src_complete(MainChannel *main_chan, int success);
> +void main_channel_on_migrate_connected(MainChannel *main_channel, gboolean
> seamless);
>  
>  #endif

Frediano