[Spice-devel] [RFC PATCH 1/2] RedChannel: Add FOREACH_CHANNEL and use it to iterate

[Uri Lublin]

On 09/16/2016 06:06 PM, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-channel.c | 50 +++++++++++++++++++++++---------------------------
>  1 file changed, 23 insertions(+), 27 deletions(-)
>

Hi Frediano,

Looks good to me.
Please make the FOREACH_CLIENT changes in a separate patch.

Thanks,
     Uri.

> diff --git a/server/red-channel.c b/server/red-channel.c
> index 474fe68..166221e 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -31,6 +31,13 @@
>  #include "main-dispatcher.h"
>  #include "utils.h"
>
> +#define FOREACH_CHANNEL(client, _link, _next, _data)                   \
> +    for (_link = (client ? (client)->channels : NULL); \
> +         (_data = (_link ? _link->data : NULL), \
> +         _next = (_link ? _link->next : NULL), \
> +         _link) != NULL; \
> +         _link = _next)
> +
>  /*
>   * Lifetime of RedChannel, RedChannelClient and RedClient:
>   * RedChannel is created and destroyed by the calls to
> @@ -483,14 +490,13 @@ void red_channel_apply_clients_data(RedChannel *channel, channel_client_callback
>
>  int red_channel_all_blocked(RedChannel *channel)
>  {
> -    GList *link;
> +    GList *link, *next;
>      RedChannelClient *rcc;
>
>      if (!channel || !channel->clients) {
>          return FALSE;
>      }
> -    for (link = channel->clients; link != NULL; link = link->next) {
> -        rcc = link->data;
> +    FOREACH_CLIENT(channel, link, next, rcc) {
>          if (!red_channel_client_is_blocked(rcc)) {
>              return FALSE;
>          }
> @@ -577,14 +583,16 @@ RedClient *red_client_unref(RedClient *client)
>
>  void red_client_set_migration_seamless(RedClient *client) // dest
>  {
> -    GList *link;
> +    GList *link, *next;
> +    RedChannelClient *rcc;
> +
>      spice_assert(client->during_target_migrate);
>      pthread_mutex_lock(&client->lock);
>      client->seamless_migrate = TRUE;
>      /* update channel clients that got connected before the migration
>       * type was set. red_client_add_channel will handle newer channel clients */
> -    for (link = client->channels; link != NULL; link = link->next) {
> -        if (red_channel_client_set_migration_seamless(link->data))
> +    FOREACH_CHANNEL(client, link, next, rcc) {
> +        if (red_channel_client_set_migration_seamless(rcc))
>              client->num_migrated_channels++;
>      }
>      pthread_mutex_unlock(&client->lock);
> @@ -603,15 +611,11 @@ void red_client_migrate(RedClient *client)
>                        " this might be a BUG",
>                        client->thread_id, pthread_self());
>      }
> -    link = client->channels;
> -    while (link) {
> -        next = link->next;
> -        rcc = link->data;
> +    FOREACH_CHANNEL(client, link, next, rcc) {
>          channel = red_channel_client_get_channel(rcc);
>          if (red_channel_client_is_connected(rcc)) {
>              channel->client_cbs.migrate(rcc);
>          }
> -        link = next;
>      }
>  }
>
> @@ -628,13 +632,10 @@ void red_client_destroy(RedClient *client)
>                        client->thread_id,
>                        pthread_self());
>      }
> -    link = client->channels;
> -    while (link) {
> +    FOREACH_CHANNEL(client, link, next, rcc) {
>          RedChannel *channel;
> -        next = link->next;
>          // some channels may be in other threads, so disconnection
>          // is not synchronous.
> -        rcc = link->data;
>          channel = red_channel_client_get_channel(rcc);
>          red_channel_client_set_destroying(rcc);
>          // some channels may be in other threads. However we currently
> @@ -646,7 +647,6 @@ void red_client_destroy(RedClient *client)
>          spice_assert(red_channel_client_pipe_is_empty(rcc));
>          spice_assert(red_channel_client_no_item_being_sent(rcc));
>          red_channel_client_destroy(rcc);
> -        link = next;
>      }
>      red_client_unref(client);
>  }
> @@ -654,13 +654,12 @@ void red_client_destroy(RedClient *client)
>  /* client->lock should be locked */
>  RedChannelClient *red_client_get_channel(RedClient *client, int type, int id)
>  {
> -    GList *link;
> +    GList *link, *next;
>      RedChannelClient *rcc;
>      RedChannelClient *ret = NULL;
>
> -    for (link = client->channels; link != NULL; link = link->next) {
> +    FOREACH_CHANNEL(client, link, next, rcc) {
>          RedChannel *channel;
> -        rcc = link->data;
>          channel = red_channel_client_get_channel(rcc);
>          if (channel->type == type && channel->id == id) {
>              ret = rcc;
> @@ -692,6 +691,7 @@ void red_client_set_main(RedClient *client, MainChannelClient *mcc) {
>  void red_client_semi_seamless_migrate_complete(RedClient *client)
>  {
>      GList *link, *next;
> +    RedChannelClient *rcc;
>
>      pthread_mutex_lock(&client->lock);
>      if (!client->during_target_migrate || client->seamless_migrate) {
> @@ -700,11 +700,8 @@ void red_client_semi_seamless_migrate_complete(RedClient *client)
>          return;
>      }
>      client->during_target_migrate = FALSE;
> -    link = client->channels;
> -    while (link) {
> -        next = link->next;
> -        red_channel_client_semi_seamless_migration_complete(link->data);
> -        link = next;
> +    FOREACH_CHANNEL(client, link, next, rcc) {
> +        red_channel_client_semi_seamless_migration_complete(rcc);
>      }
>      pthread_mutex_unlock(&client->lock);
>      reds_on_client_semi_seamless_migrate_complete(client->reds, client);
> @@ -790,13 +787,12 @@ void red_channel_pipes_new_add_tail(RedChannel *channel, new_pipe_item_t creator
>
>  uint32_t red_channel_max_pipe_size(RedChannel *channel)
>  {
> -    GList *link;
> +    GList *link, *next;
>      RedChannelClient *rcc;
>      uint32_t pipe_size = 0;
>
> -    for (link = channel->clients; link != NULL; link = link->next) {
> +    FOREACH_CLIENT(channel, link, next, rcc) {
>          uint32_t new_size;
> -        rcc = link->data;
>          new_size = red_channel_client_get_pipe_size(rcc);
>          pipe_size = MAX(pipe_size, new_size);
>      }
>