[Spice-devel] [PATCH 05/10] Replace a couple Rings with GList

[Frediano Ziglio]

> 
> On Thu, Sep 08, 2016 at 11:52:55AM -0500, Jonathon Jongsma wrote:
> > Make RedsState::mig_target_clients into a GList to improve encapsulation
> > and maintainability. Also RedsMigTargetClient::pending_links. With
> > GList, a type implementation can be ignorant of whether they're
> > contained within a list or not.
> > ---
> >  server/reds-private.h |  6 ++----
> >  server/reds.c         | 57
> >  ++++++++++++++++++++++-----------------------------
> >  2 files changed, 27 insertions(+), 36 deletions(-)
> > 
> > diff --git a/server/reds-private.h b/server/reds-private.h
> > index 0408f25..29645bf 100644
> > --- a/server/reds-private.h
> > +++ b/server/reds-private.h
> > @@ -61,16 +61,14 @@ typedef struct RedsStatValue {
> >  #endif
> >  
> >  typedef struct RedsMigPendingLink {
> > -    RingItem ring_link; // list of links that belongs to the same client
> >      SpiceLinkMess *link_msg;
> >      RedsStream *stream;
> >  } RedsMigPendingLink;
> >  
> >  typedef struct RedsMigTargetClient {
> >      RedsState *reds;
> > -    RingItem link;
> >      RedClient *client;
> > -    Ring pending_links;
> > +    GList *pending_links;
> >  } RedsMigTargetClient;
> >  
> >  /* Intermediate state for on going monitors config message from a single
> > @@ -122,7 +120,7 @@ struct RedsState {
> >                                      between the 2 servers */
> >      int dst_do_seamless_migrate; /* per migration. Updated after the
> >      migration handshake
> >                                      between the 2 servers */
> > -    Ring mig_target_clients;
> > +    GList *mig_target_clients;
> >  
> >      int num_of_channels;
> >      Ring channels;
> > diff --git a/server/reds.c b/server/reds.c
> > index 81b378c..153e201 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -295,7 +295,7 @@ static RedCharDeviceVDIPort
> > *red_char_device_vdi_port_new(RedsState *reds);
> >  
> >  static void migrate_timeout(void *opaque);
> >  static RedsMigTargetClient* reds_mig_target_client_find(RedsState *reds,
> >  RedClient *client);
> > -static void reds_mig_target_client_free(RedsMigTargetClient *mig_client);
> > +static void reds_mig_target_client_free(RedsState *reds,
> > RedsMigTargetClient *mig_client);
> >  static void reds_mig_cleanup_wait_disconnect(RedsState *reds);
> >  static void reds_mig_remove_wait_disconnect_client(RedsState *reds,
> >  RedClient *client);
> >  static void reds_add_char_device(RedsState *reds, RedCharDevice *dev);
> > @@ -596,7 +596,7 @@ void reds_client_disconnect(RedsState *reds, RedClient
> > *client)
> >  
> >      mig_client = reds_mig_target_client_find(reds, client);
> >      if (mig_client) {
> > -        reds_mig_target_client_free(mig_client);
> > +        reds_mig_target_client_free(reds, mig_client);
> >      }
> >  
> >      if (reds->mig_wait_disconnect) {
> > @@ -1678,24 +1678,22 @@ static void reds_mig_target_client_add(RedsState
> > *reds, RedClient *client)
> >  {
> >      RedsMigTargetClient *mig_client;
> >  
> > -    spice_assert(reds);
> > +    g_return_if_fail(reds);
> 
> Not strictly related to that commit, but why not.
> 
> >      spice_info(NULL);
> > -    mig_client = spice_malloc0(sizeof(RedsMigTargetClient));
> > +    mig_client = g_new0(RedsMigTargetClient, 1);
> >      mig_client->client = client;
> >      mig_client->reds = reds;
> > -    ring_init(&mig_client->pending_links);
> > -    ring_add(&reds->mig_target_clients, &mig_client->link);
> > -
> > +    mig_client->pending_links = NULL;
> 
> Not really needed as a few lines above you have:
> mig_client = g_new0(RedsMigTargetClient, 1);
> 

I would personal refrain from change memory allocator now.

> > +    reds->mig_target_clients = g_list_append(reds->mig_target_clients,
> > mig_client);
> >  }
> >  
> >  static RedsMigTargetClient* reds_mig_target_client_find(RedsState *reds,
> >  RedClient *client)
> >  {
> > -    RingItem *item;
> > +    GList *l;
> >  
> > -    RING_FOREACH(item, &reds->mig_target_clients) {
> > -        RedsMigTargetClient *mig_client;
> > +    for (l = reds->mig_target_clients; l != NULL; l = l->next) {
> > +        RedsMigTargetClient *mig_client = l->data;
> >  
> > -        mig_client = SPICE_CONTAINEROF(item, RedsMigTargetClient, link);
> >          if (mig_client->client == client) {
> >              return mig_client;
> >          }
> > @@ -1710,34 +1708,30 @@ static void
> > reds_mig_target_client_add_pending_link(RedsMigTargetClient *client,
> >      RedsMigPendingLink *mig_link;
> >  
> >      spice_assert(client);
> > -    mig_link = spice_malloc0(sizeof(RedsMigPendingLink));
> > +    mig_link = g_new0(RedsMigPendingLink, 1);

Same here.

> >      mig_link->link_msg = link_msg;
> >      mig_link->stream = stream;
> >  
> > -    ring_add(&client->pending_links, &mig_link->ring_link);
> > +    client->pending_links = g_list_append(client->pending_links,
> > mig_link);
> >  }
> >  
> > -static void reds_mig_target_client_free(RedsMigTargetClient *mig_client)
> > +static void reds_mig_target_client_free(RedsState *reds,
> > RedsMigTargetClient *mig_client)
> >  {
> > -    RingItem *now, *next;
> > -
> > -    ring_remove(&mig_client->link);
> > -
> > -    RING_FOREACH_SAFE(now, next, &mig_client->pending_links) {
> > -        RedsMigPendingLink *mig_link = SPICE_CONTAINEROF(now,
> > RedsMigPendingLink, ring_link);
> > -        ring_remove(now);
> > -        free(mig_link);
> > -    }
> > +    reds->mig_target_clients = g_list_remove(reds->mig_target_clients,
> > mig_client);
> > +    g_list_free_full(mig_client->pending_links, g_free);
> >      free(mig_client);
> 
> Probably should be changed to g_free()
> 

Yes, this is a good reason why not changing now.

> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 

Frediano