[Spice-devel] [PATCH v2 9/9] Change RedCharDevicePrivate::clients to GList

[Jonathon Jongsma]

On Thu, 2016-09-15 at 11:16 -0400, Frediano Ziglio wrote:
> > 
> > 
> > More Ring cleanup. At the moment, we only support a single client,
> > so
> > this is only a one-element list
> > ---
> > Changes in v2:
> >  - simplified red_char_device_finalize() to avoid using
> > g_list_last()
> > 
> >  server/char-device.c | 68
> >  ++++++++++++++++++++--------------------------------
> >  1 file changed, 26 insertions(+), 42 deletions(-)
> > 
> > diff --git a/server/char-device.c b/server/char-device.c
> > index f826524..1b99aa9 100644
> > --- a/server/char-device.c
> > +++ b/server/char-device.c
> > @@ -32,7 +32,6 @@
> >  
> >  typedef struct RedCharDeviceClient RedCharDeviceClient;
> >  struct RedCharDeviceClient {
> > -    RingItem link;
> >      RedCharDevice *dev;
> >      RedClient *client;
> >      int do_flow_control;
> > @@ -58,8 +57,7 @@ struct RedCharDevicePrivate {
> >      SpiceTimer *write_to_dev_timer;
> >      uint64_t num_self_tokens;
> >  
> > -    Ring clients; /* list of RedCharDeviceClient */
> > -    uint32_t num_clients;
> > +    GList *clients; /* list of RedCharDeviceClient */
> >  
> >      uint64_t client_tokens_interval; /* frequency of returning
> > tokens to the
> >      client */
> >      SpiceCharDeviceInstance *sin;
> > @@ -207,8 +205,7 @@ static void
> > red_char_device_client_free(RedCharDevice
> > *dev,
> >          dev->priv->cur_write_buf->client = NULL;
> >      }
> >  
> > -    dev->priv->num_clients--;
> > -    ring_remove(&dev_client->link);
> > +    dev->priv->clients = g_list_remove(dev->priv->clients,
> > dev_client);
> >      free(dev_client);
> >  }
> >  
> > @@ -222,12 +219,11 @@ static void
> > red_char_device_handle_client_overflow(RedCharDeviceClient
> > *dev_clie
> >  static RedCharDeviceClient
> > *red_char_device_client_find(RedCharDevice *dev,
> >                                                          RedClient
> > *client)
> >  {
> > -    RingItem *item;
> > +    GList *item;
> >  
> > -    RING_FOREACH(item, &dev->priv->clients) {
> > -        RedCharDeviceClient *dev_client;
> > +    for (item = dev->priv->clients; item != NULL; item = item-
> > >next) {
> > +        RedCharDeviceClient *dev_client = item->data;
> >  
> > -        dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient,
> > link);
> >          if (dev_client->client == client) {
> >              return dev_client;
> >          }
> > @@ -253,13 +249,11 @@ static int
> > red_char_device_can_send_to_client(RedCharDeviceClient *dev_client)
> >  
> >  static uint64_t red_char_device_max_send_tokens(RedCharDevice
> > *dev)
> >  {
> > -    RingItem *item;
> > +    GList *item;
> >      uint64_t max = 0;
> >  
> > -    RING_FOREACH(item, &dev->priv->clients) {
> > -        RedCharDeviceClient *dev_client;
> > -
> > -        dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient,
> > link);
> > +    for (item = dev->priv->clients; item != NULL; item = item-
> > >next) {
> > +        RedCharDeviceClient *dev_client = item->data;
> >  
> >          if (!dev_client->do_flow_control) {
> >              max = ~0;
> > @@ -295,12 +289,11 @@ static void
> > red_char_device_add_msg_to_client_queue(RedCharDeviceClient
> > *dev_cli
> >  static void red_char_device_send_msg_to_clients(RedCharDevice
> > *dev,
> >                                                  RedPipeItem *msg)
> >  {
> > -    RingItem *item, *next;
> > +    GList *l;
> >  
> > -    RING_FOREACH_SAFE(item, next, &dev->priv->clients) {
> > -        RedCharDeviceClient *dev_client;
> > +    for (l = dev->priv->clients; l != NULL; l = l->next) {
> > +        RedCharDeviceClient *dev_client = l->data;
> >  
> > -        dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient,
> > link);
> >          if (red_char_device_can_send_to_client(dev_client)) {
> >              dev_client->num_send_tokens--;
> >              spice_assert(g_queue_is_empty(dev_client-
> > >send_queue));
> 
> The _SAFE loop here was necessary as there are combinations were
> the client is removed from the list during the loop.
> This will make l vanish before l->next is read.
> Even if there is only a client this can happen.

Hmm, I couldn't find a code path where this happened. But there is a
comment within the loop suggesting that it can happen, so it's better
to use the safer loop style. Sorry for changing it without enough
diligence.

jonathon