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

[Frediano Ziglio]

> 
> > 
> > Typo in commit summary (CHANNELL -> CHANNEL)
> > 
> > Also: do we really need an unsafe version of the loop? why not just have a
> > single version that's always safe?
> > 
> 
> That's fine for me, just in the code you respected the safe/unsafe
> behavior.
> 
> Actually I think can be rewritten to
> 
> #define FOREACH_CHANNEL(client, _link, _next, _data) \
>     for (_link = (client ? (client)->channels : NULL), \
>         _next = (_link ? _link->next : NULL); \
>         (_data = (_link ? _link->data : NULL)) != NULL; \
>         _link = _next, \
>         _next = (_link ? _link->next : NULL))
> 
> or even
> 
> #define FOREACH_CHANNEL(client, _link, _data) \
>     for (_link = (client ? (client)->channels : NULL); \
>          (_data = (_link ? _link->data : NULL), \
>          _link = (_link ? _link->next : NULL), \
>          _data) != NULL; )
> 

No, you lose the possibility to use the link to remove item
faster (you can get from next but it's confusing), better

#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)

Frediano

> 
> > 
> > On Mon, 2016-05-23 at 12:42 +0100, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/red-channel.c | 44 ++++++++++++++++++++++++--------------------
> > >  1 file changed, 24 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/server/red-channel.c b/server/red-channel.c
> > > index 2b37fed..9ec9a26 100644
> > > --- a/server/red-channel.c
> > > +++ b/server/red-channel.c
> > > @@ -59,6 +59,21 @@ typedef struct MarkerPipeItem {
> > >  
> > >  #define CHANNEL_BLOCKED_SLEEP_DURATION 10000 //micro
> > >  
> > > +#define FOREACH_CHANNEL(client, _link, _data)                   \
> > > +    for (_link = (client ? (client)->channels : NULL); \
> > > +         (_data = (_link ? _link->data : NULL), \
> > > +         _link) != NULL; \
> > > +         _link = _link->next)
> > > +
> > > +#define FOREACH_CHANNEL_SAFE(client, _link, _next, _data)
> > > \
> > > +    for (_link = (client ? (client)->channels : NULL), \
> > > +         _next = (_link ? _link->next : NULL), \
> > > +         _data = (_link ? _link->data : NULL); \
> > > +         _link; \
> > > +         _link = _next, \
> > > +         _next = (_link ? _link->next : NULL), \
> > > +         _data = (_link ? _link->data : NULL))
> > > +
> > >  enum QosPingState {
> > >      PING_STATE_NONE,
> > >      PING_STATE_TIMER,
> > > @@ -2053,13 +2068,15 @@ static gboolean
> > > red_channel_client_set_migration_seamless(RedChannelClient *rcc)
> > >  void red_client_set_migration_seamless(RedClient *client) // dest
> > >  {
> > >      GList *link;
> > > +    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, rcc) {
> > > +        if (red_channel_client_set_migration_seamless(rcc))
> > >              client->num_migrated_channels++;
> > >      }
> > >      pthread_mutex_unlock(&client->lock);
> > > @@ -2077,14 +2094,10 @@ 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_SAFE(client, link, next, rcc) {
> > >          if (red_channel_client_is_connected(rcc)) {
> > >              rcc->channel->client_cbs.migrate(rcc);
> > >          }
> > > -        link = next;
> > >      }
> > >  }
> > >  
> > > @@ -2101,12 +2114,9 @@ void red_client_destroy(RedClient *client)
> > >                        client->thread_id,
> > >                        pthread_self());
> > >      }
> > > -    link = client->channels;
> > > -    while (link) {
> > > -        next = link->next;
> > > +    FOREACH_CHANNEL_SAFE(client, link, next, rcc) {
> > >          // some channels may be in other threads, so disconnection
> > >          // is not synchronous.
> > > -        rcc = link->data;
> > >          rcc->destroying = 1;
> > >          // some channels may be in other threads. However we currently
> > >          // assume disconnect is synchronous (we changed the dispatcher
> > > @@ -2118,7 +2128,6 @@ void red_client_destroy(RedClient *client)
> > >          spice_assert(rcc->pipe_size == 0);
> > >          spice_assert(rcc->send_data.size == 0);
> > >          red_channel_client_destroy(rcc);
> > > -        link = next;
> > >      }
> > >      red_client_unref(client);
> > >  }
> > > @@ -2130,8 +2139,7 @@ static RedChannelClient
> > > *red_client_get_channel(RedClient *client, int type, int
> > >      RedChannelClient *rcc;
> > >      RedChannelClient *ret = NULL;
> > >  
> > > -    for (link = client->channels; link != NULL; link = link->next) {
> > > -        rcc = link->data;
> > > +    FOREACH_CHANNEL(client, link, rcc) {
> > >          if (rcc->channel->type == type && rcc->channel->id == id) {
> > >              ret = rcc;
> > >              break;
> > > @@ -2162,6 +2170,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) {
> > > @@ -2170,15 +2179,10 @@ void
> > > red_client_semi_seamless_migrate_complete(RedClient *client)
> > >          return;
> > >      }
> > >      client->during_target_migrate = FALSE;
> > > -    link = client->channels;
> > > -    while (link) {
> > > -        next = link->next;
> > > -        RedChannelClient *rcc = link->data;
> > > -
> > > +    FOREACH_CHANNEL_SAFE(client, link, next, rcc) {
> > >          if (rcc->latency_monitor.timer) {
> > >              red_channel_client_start_ping_timer(rcc,
> > > PING_TEST_IDLE_NET_TIMEOUT_MS);
> > >          }
> > > -        link = next;
> > >      }
> > >      pthread_mutex_unlock(&client->lock);
> > >      reds_on_client_semi_seamless_migrate_complete(client->reds, client);
> > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>