[Spice-devel] [PATCH 05/12] char-device: Make SpiceCharDeviceState a gobject

Frediano Ziglio fziglio at redhat.com
Mon Mar 21 18:12:49 UTC 2016


Just for coherence I would change the case from gobject to GObject

> 
> From: Christophe Fergeau <cfergeau at redhat.com>
> 
> ---
>  server/char-device.c | 586
>  +++++++++++++++++++++++++++++++++------------------
>  server/char-device.h |  38 +++-
>  2 files changed, 422 insertions(+), 202 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index d2f1ea3..a74626a 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -1,6 +1,6 @@
>  /* spice-server char device flow control code
>  
> -   Copyright (C) 2012 Red Hat, Inc.
> +   Copyright (C) 2012-2015 Red Hat, Inc.
>  
>     Red Hat Authors:
>     Yonit Halperin <yhalperi at redhat.com>
> @@ -46,7 +46,7 @@ struct SpiceCharDeviceClientState {
>      uint32_t max_send_queue_size;
>  };
>  
> -struct SpiceCharDeviceState {
> +struct RedCharDevicePrivate {
>      int running;
>      int active; /* has read/write been performed since the device was
>      started */
>      int wait_for_migrate_data;

Why this name change (Spice -> Red) ?

> @@ -81,10 +81,27 @@ enum {
>      WRITE_BUFFER_ORIGIN_SERVER_NO_TOKEN,
>  };
>  
> +
> +G_DEFINE_TYPE(RedCharDevice, red_char_device, G_TYPE_OBJECT)
> +
> +#define RED_CHAR_DEVICE_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o),
> RED_TYPE_CHAR_DEVICE, RedCharDevicePrivate))
> +
> +enum {
> +    PROP_0,
> +    PROP_CHAR_DEV_INSTANCE,
> +    PROP_SPICE_SERVER,
> +    PROP_CLIENT_TOKENS_INTERVAL,
> +    PROP_SELF_TOKENS,
> +    PROP_OPAQUE
> +};
> +
>  /* Holding references for avoiding access violation if the char device was
>   * destroyed during a callback */
> -static void spice_char_device_state_ref(SpiceCharDeviceState *char_dev);
> -static void spice_char_device_state_unref(SpiceCharDeviceState *char_dev);
> +#define spice_char_device_state_ref g_object_ref
> +/* refcounting is used to protect the char_dev from being deallocated in
> + * case spice_char_device_state_destroy has been called
> + * during a callback, and we might still access the char_dev afterwards. */
> +#define spice_char_device_state_unref g_object_unref

I would personally use some static inline functions.

>  static void spice_char_device_write_buffer_unref(SpiceCharDeviceWriteBuffer
>  *write_buf);
>  
>  static void spice_char_dev_write_retry(void *opaque);
> @@ -94,13 +111,20 @@ typedef struct SpiceCharDeviceMsgToClientItem {
>      SpiceCharDeviceMsgToClient *msg;
>  } SpiceCharDeviceMsgToClientItem;
>  
> +static RedCharDevice *red_char_device_new(SpiceCharDeviceInstance *sin,
> +                                          RedsState *reds,
> +                                          uint32_t client_tokens_interval,
> +                                          uint32_t self_tokens,
> +                                          SpiceCharDeviceCallbacks *cbs,
> +                                          void *opaque);
> +
>  static SpiceCharDeviceMsgToClient *
>  spice_char_device_read_one_msg_from_device(SpiceCharDeviceState *dev)
>  {
>     g_return_val_if_fail(dev != NULL, NULL);
> -   g_return_val_if_fail(dev->cbs.read_one_msg_from_device != NULL, NULL);
> +   g_return_val_if_fail(dev->priv->cbs.read_one_msg_from_device != NULL,
> NULL);
>  
> -   return dev->cbs.read_one_msg_from_device(dev->sin, dev->opaque);
> +   return dev->priv->cbs.read_one_msg_from_device(dev->priv->sin,
> dev->priv->opaque);
>  }
>  
>  static SpiceCharDeviceMsgToClient *
> @@ -108,9 +132,9 @@ spice_char_device_ref_msg_to_client(SpiceCharDeviceState
> *dev,
>                                      SpiceCharDeviceMsgToClient *msg)
>  {
>     g_return_val_if_fail(dev != NULL, NULL);
> -   g_return_val_if_fail(dev->cbs.ref_msg_to_client != NULL, NULL);
> +   g_return_val_if_fail(dev->priv->cbs.ref_msg_to_client != NULL, NULL);
>  
> -   return dev->cbs.ref_msg_to_client(msg, dev->opaque);
> +   return dev->priv->cbs.ref_msg_to_client(msg, dev->priv->opaque);
>  }
>  
>  static void
> @@ -118,9 +142,9 @@
> spice_char_device_unref_msg_to_client(SpiceCharDeviceState *dev,
>                                        SpiceCharDeviceMsgToClient *msg)
>  {
>     g_return_if_fail(dev != NULL);
> -   g_return_if_fail(dev->cbs.unref_msg_to_client != NULL);
> +   g_return_if_fail(dev->priv->cbs.unref_msg_to_client != NULL);
>  
> -   dev->cbs.unref_msg_to_client(msg, dev->opaque);
> +   dev->priv->cbs.unref_msg_to_client(msg, dev->priv->opaque);
>  }
>  
>  static void
> @@ -129,9 +153,9 @@ spice_char_device_send_msg_to_client(SpiceCharDeviceState
> *dev,
>                                       RedClient *client)
>  {
>     g_return_if_fail(dev != NULL);
> -   g_return_if_fail(dev->cbs.send_msg_to_client != NULL);
> +   g_return_if_fail(dev->priv->cbs.send_msg_to_client != NULL);
>  
> -   dev->cbs.send_msg_to_client(msg, client, dev->opaque);
> +   dev->priv->cbs.send_msg_to_client(msg, client, dev->priv->opaque);
>  }
>  
>  static void
> @@ -140,17 +164,17 @@
> spice_char_device_send_tokens_to_client(SpiceCharDeviceState *dev,
>                                          uint32_t tokens)
>  {
>     g_return_if_fail(dev != NULL);
> -   g_return_if_fail(dev->cbs.send_tokens_to_client != NULL);
> +   g_return_if_fail(dev->priv->cbs.send_tokens_to_client != NULL);
>  
> -   dev->cbs.send_tokens_to_client(client, tokens, dev->opaque);
> +   dev->priv->cbs.send_tokens_to_client(client, tokens, dev->priv->opaque);
>  }
>  
>  static void
>  spice_char_device_on_free_self_token(SpiceCharDeviceState *dev)
>  {
>     g_return_if_fail(dev != NULL);
> -   if (dev->cbs.on_free_self_token != NULL) {
> -       dev->cbs.on_free_self_token(dev->opaque);
> +   if (dev->priv->cbs.on_free_self_token != NULL) {
> +       dev->priv->cbs.on_free_self_token(dev->priv->opaque);
>     }
>  }
>  
> @@ -158,9 +182,9 @@ static void
>  spice_char_device_remove_client(SpiceCharDeviceState *dev, RedClient
>  *client)
>  {
>     g_return_if_fail(dev != NULL);
> -   g_return_if_fail(dev->cbs.remove_client != NULL);
> +   g_return_if_fail(dev->priv->cbs.remove_client != NULL);
>  
> -   dev->cbs.remove_client(client, dev->opaque);
> +   dev->priv->cbs.remove_client(client, dev->priv->opaque);
>  }
>  
>  static void spice_char_device_write_buffer_free(SpiceCharDeviceWriteBuffer
>  *buf)
> @@ -188,12 +212,12 @@ static void
> spice_char_device_write_buffer_pool_add(SpiceCharDeviceState *dev,
>                                                      SpiceCharDeviceWriteBuffer
>                                                      *buf)
>  {
>      if (buf->refs == 1 &&
> -        dev->cur_pool_size < MAX_POOL_SIZE) {
> +        dev->priv->cur_pool_size < MAX_POOL_SIZE) {
>          buf->buf_used = 0;
>          buf->origin = WRITE_BUFFER_ORIGIN_NONE;
>          buf->client = NULL;
> -        dev->cur_pool_size += buf->buf_size;
> -        ring_add(&dev->write_bufs_pool, &buf->link);
> +        dev->priv->cur_pool_size += buf->buf_size;
> +        ring_add(&dev->priv->write_bufs_pool, &buf->link);
>          return;
>      }
>  
> @@ -225,14 +249,15 @@ static void
> spice_char_device_client_free(SpiceCharDeviceState *dev,
>      RingItem *item, *next;
>  
>      if (dev_client->wait_for_tokens_timer) {
> -        reds_core_timer_remove(dev->reds,
> dev_client->wait_for_tokens_timer);
> +        reds_core_timer_remove(dev->priv->reds,
> dev_client->wait_for_tokens_timer);
> +        dev_client->wait_for_tokens_timer = NULL;

I like this change but it's hidden by all priv changes.
Perhaps better to put in a separate patch.

>      }
>  
>      spice_char_device_client_send_queue_free(dev, dev_client);
>  
>      /* remove write buffers that are associated with the client */
> -    spice_debug("write_queue_is_empty %d", ring_is_empty(&dev->write_queue)
> && !dev->cur_write_buf);
> -    RING_FOREACH_SAFE(item, next, &dev->write_queue) {
> +    spice_debug("write_queue_is_empty %d",
> ring_is_empty(&dev->priv->write_queue) && !dev->priv->cur_write_buf);
> +    RING_FOREACH_SAFE(item, next, &dev->priv->write_queue) {
>          SpiceCharDeviceWriteBuffer *write_buf;
>  
>          write_buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer,
>          link);
> @@ -243,13 +268,13 @@ static void
> spice_char_device_client_free(SpiceCharDeviceState *dev,
>          }
>      }
>  
> -    if (dev->cur_write_buf && dev->cur_write_buf->origin ==
> WRITE_BUFFER_ORIGIN_CLIENT &&
> -        dev->cur_write_buf->client == dev_client->client) {
> -        dev->cur_write_buf->origin = WRITE_BUFFER_ORIGIN_NONE;
> -        dev->cur_write_buf->client = NULL;
> +    if (dev->priv->cur_write_buf && dev->priv->cur_write_buf->origin ==
> WRITE_BUFFER_ORIGIN_CLIENT &&
> +        dev->priv->cur_write_buf->client == dev_client->client) {
> +        dev->priv->cur_write_buf->origin = WRITE_BUFFER_ORIGIN_NONE;
> +        dev->priv->cur_write_buf->client = NULL;
>      }
>  
> -    dev->num_clients--;
> +    dev->priv->num_clients--;
>      ring_remove(&dev_client->link);
>      free(dev_client);
>  }
> @@ -266,7 +291,7 @@ static SpiceCharDeviceClientState
> *spice_char_device_client_find(SpiceCharDevice
>  {
>      RingItem *item;
>  
> -    RING_FOREACH(item, &dev->clients) {
> +    RING_FOREACH(item, &dev->priv->clients) {
>          SpiceCharDeviceClientState *dev_client;
>  
>          dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState,
>          link);
> @@ -298,7 +323,7 @@ static uint64_t
> spice_char_device_max_send_tokens(SpiceCharDeviceState *dev)
>      RingItem *item;
>      uint64_t max = 0;
>  
> -    RING_FOREACH(item, &dev->clients) {
> +    RING_FOREACH(item, &dev->priv->clients) {
>          SpiceCharDeviceClientState *dev_client;
>  
>          dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState,
>          link);
> @@ -331,7 +356,7 @@ static void
> spice_char_device_add_msg_to_client_queue(SpiceCharDeviceClientState
>      ring_add(&dev_client->send_queue, &msg_item->link);
>      dev_client->send_queue_size++;
>      if (!dev_client->wait_for_tokens_started) {
> -        reds_core_timer_start(dev->reds, dev_client->wait_for_tokens_timer,
> +        reds_core_timer_start(dev->priv->reds,
> dev_client->wait_for_tokens_timer,
>                                SPICE_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
>          dev_client->wait_for_tokens_started = TRUE;
>      }
> @@ -342,7 +367,7 @@ static void
> spice_char_device_send_msg_to_clients(SpiceCharDeviceState *dev,
>  {
>      RingItem *item, *next;
>  
> -    RING_FOREACH_SAFE(item, next, &dev->clients) {
> +    RING_FOREACH_SAFE(item, next, &dev->priv->clients) {
>          SpiceCharDeviceClientState *dev_client;
>  
>          dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState,
>          link);
> @@ -363,7 +388,7 @@ static int
> spice_char_device_read_from_device(SpiceCharDeviceState *dev)
>      uint64_t max_send_tokens;
>      int did_read = FALSE;
>  
> -    if (!dev->running || dev->wait_for_migrate_data || !dev->sin) {
> +    if (!dev->priv->running || dev->priv->wait_for_migrate_data ||
> !dev->priv->sin) {
>          return FALSE;
>      }
>  
> @@ -373,7 +398,7 @@ static int
> spice_char_device_read_from_device(SpiceCharDeviceState *dev)
>       * 2) in case of sending messages to the client, and unreferencing the
>       * msg, we trigger another read.
>       */
> -    if (dev->during_read_from_device++ > 0) {
> +    if (dev->priv->during_read_from_device++ > 0) {
>          return FALSE;
>      }
>  
> @@ -383,13 +408,13 @@ static int
> spice_char_device_read_from_device(SpiceCharDeviceState *dev)
>       * Reading from the device only in case at least one of the clients have
>       a free token.
>       * All messages will be discarded if no client is attached to the device
>       */
> -    while ((max_send_tokens || ring_is_empty(&dev->clients)) &&
> dev->running) {
> +    while ((max_send_tokens || ring_is_empty(&dev->priv->clients)) &&
> dev->priv->running) {
>          SpiceCharDeviceMsgToClient *msg;
>  
>          msg = spice_char_device_read_one_msg_from_device(dev);
>          if (!msg) {
> -            if (dev->during_read_from_device > 1) {
> -                dev->during_read_from_device = 1;
> +            if (dev->priv->during_read_from_device > 1) {
> +                dev->priv->during_read_from_device = 1;
>                  continue; /* a wakeup might have been called during the read
>                  -
>                               make sure it doesn't get lost */
>              }
> @@ -400,9 +425,9 @@ static int
> spice_char_device_read_from_device(SpiceCharDeviceState *dev)
>          spice_char_device_unref_msg_to_client(dev, msg);
>          max_send_tokens--;
>      }
> -    dev->during_read_from_device = 0;
> -    if (dev->running) {
> -        dev->active = dev->active || did_read;
> +    dev->priv->during_read_from_device = 0;
> +    if (dev->priv->running) {
> +        dev->priv->active = dev->priv->active || did_read;
>      }
>      spice_char_device_state_unref(dev);
>      return did_read;
> @@ -440,11 +465,11 @@ static void
> spice_char_device_send_to_client_tokens_absorb(SpiceCharDeviceClient
>      }
>  
>      if (spice_char_device_can_send_to_client(dev_client)) {
> -        reds_core_timer_cancel(dev->reds,
> dev_client->wait_for_tokens_timer);
> +        reds_core_timer_cancel(dev->priv->reds,
> dev_client->wait_for_tokens_timer);
>          dev_client->wait_for_tokens_started = FALSE;
>          spice_char_device_read_from_device(dev_client->dev);
>      } else if (dev_client->send_queue_size) {
> -        reds_core_timer_start(dev->reds, dev_client->wait_for_tokens_timer,
> +        reds_core_timer_start(dev->priv->reds,
> dev_client->wait_for_tokens_timer,
>                                SPICE_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
>          dev_client->wait_for_tokens_started = TRUE;
>      }
> @@ -497,7 +522,7 @@ static void
> spice_char_device_client_tokens_add(SpiceCharDeviceState *dev,
>          spice_debug("#tokens > 1 (=%u)", num_tokens);
>      }
>      dev_client->num_client_tokens_free += num_tokens;
> -    if (dev_client->num_client_tokens_free >= dev->client_tokens_interval) {
> +    if (dev_client->num_client_tokens_free >=
> dev->priv->client_tokens_interval) {
>          uint32_t tokens = dev_client->num_client_tokens_free;
>  
>          dev_client->num_client_tokens += dev_client->num_client_tokens_free;
> @@ -512,41 +537,41 @@ static int
> spice_char_device_write_to_device(SpiceCharDeviceState *dev)
>      int total = 0;
>      int n;
>  
> -    if (!dev->running || dev->wait_for_migrate_data || !dev->sin) {
> +    if (!dev->priv->running || dev->priv->wait_for_migrate_data ||
> !dev->priv->sin) {
>          return 0;
>      }
>  
>      /* protect against recursion with spice_char_device_wakeup */
> -    if (dev->during_write_to_device++ > 0) {
> +    if (dev->priv->during_write_to_device++ > 0) {
>          return 0;
>      }
>  
>      spice_char_device_state_ref(dev);
>  
> -    if (dev->write_to_dev_timer) {
> -        reds_core_timer_cancel(dev->reds, dev->write_to_dev_timer);
> +    if (dev->priv->write_to_dev_timer) {
> +        reds_core_timer_cancel(dev->priv->reds,
> dev->priv->write_to_dev_timer);
>      }
>  
> -    sif = spice_char_device_get_interface(dev->sin);
> -    while (dev->running) {
> +    sif = spice_char_device_get_interface(dev->priv->sin);
> +    while (dev->priv->running) {
>          uint32_t write_len;
>  
> -        if (!dev->cur_write_buf) {
> -            RingItem *item = ring_get_tail(&dev->write_queue);
> +        if (!dev->priv->cur_write_buf) {
> +            RingItem *item = ring_get_tail(&dev->priv->write_queue);
>              if (!item) {
>                  break;
>              }
> -            dev->cur_write_buf = SPICE_CONTAINEROF(item,
> SpiceCharDeviceWriteBuffer, link);
> -            dev->cur_write_buf_pos = dev->cur_write_buf->buf;
> +            dev->priv->cur_write_buf = SPICE_CONTAINEROF(item,
> SpiceCharDeviceWriteBuffer, link);
> +            dev->priv->cur_write_buf_pos = dev->priv->cur_write_buf->buf;
>              ring_remove(item);
>          }
>  
> -        write_len = dev->cur_write_buf->buf + dev->cur_write_buf->buf_used -
> -                    dev->cur_write_buf_pos;
> -        n = sif->write(dev->sin, dev->cur_write_buf_pos, write_len);
> +        write_len = dev->priv->cur_write_buf->buf +
> dev->priv->cur_write_buf->buf_used -
> +                    dev->priv->cur_write_buf_pos;
> +        n = sif->write(dev->priv->sin, dev->priv->cur_write_buf_pos,
> write_len);
>          if (n <= 0) {
> -            if (dev->during_write_to_device > 1) {
> -                dev->during_write_to_device = 1;
> +            if (dev->priv->during_write_to_device > 1) {
> +                dev->priv->during_write_to_device = 1;
>                  continue; /* a wakeup might have been called during the
>                  write -
>                               make sure it doesn't get lost */
>              }
> @@ -555,26 +580,26 @@ static int
> spice_char_device_write_to_device(SpiceCharDeviceState *dev)
>          total += n;
>          write_len -= n;
>          if (!write_len) {
> -            SpiceCharDeviceWriteBuffer *release_buf = dev->cur_write_buf;
> -            dev->cur_write_buf = NULL;
> +            SpiceCharDeviceWriteBuffer *release_buf =
> dev->priv->cur_write_buf;
> +            dev->priv->cur_write_buf = NULL;
>              spice_char_device_write_buffer_release(dev, release_buf);
>              continue;
>          }
> -        dev->cur_write_buf_pos += n;
> +        dev->priv->cur_write_buf_pos += n;
>      }
>      /* retry writing as long as the write queue is not empty */
> -    if (dev->running) {
> -        if (dev->cur_write_buf) {
> -            if (dev->write_to_dev_timer) {
> -                reds_core_timer_start(dev->reds, dev->write_to_dev_timer,
> +    if (dev->priv->running) {
> +        if (dev->priv->cur_write_buf) {
> +            if (dev->priv->write_to_dev_timer) {
> +                reds_core_timer_start(dev->priv->reds,
> dev->priv->write_to_dev_timer,
>                                        CHAR_DEVICE_WRITE_TO_TIMEOUT);
>              }
>          } else {
> -            spice_assert(ring_is_empty(&dev->write_queue));
> +            spice_assert(ring_is_empty(&dev->priv->write_queue));
>          }
> -        dev->active = dev->active || total;
> +        dev->priv->active = dev->priv->active || total;
>      }
> -    dev->during_write_to_device = 0;
> +    dev->priv->during_write_to_device = 0;
>      spice_char_device_state_unref(dev);
>      return total;
>  }
> @@ -583,8 +608,8 @@ static void spice_char_dev_write_retry(void *opaque)
>  {
>      SpiceCharDeviceState *dev = opaque;
>  
> -    if (dev->write_to_dev_timer) {
> -        reds_core_timer_cancel(dev->reds, dev->write_to_dev_timer);
> +    if (dev->priv->write_to_dev_timer) {
> +        reds_core_timer_cancel(dev->priv->reds,
> dev->priv->write_to_dev_timer);
>      }
>      spice_char_device_write_to_device(dev);
>  }
> @@ -596,14 +621,14 @@ static SpiceCharDeviceWriteBuffer
> *__spice_char_device_write_buffer_get(
>      RingItem *item;
>      SpiceCharDeviceWriteBuffer *ret;
>  
> -    if (origin == WRITE_BUFFER_ORIGIN_SERVER && !dev->num_self_tokens) {
> +    if (origin == WRITE_BUFFER_ORIGIN_SERVER && !dev->priv->num_self_tokens)
> {
>          return NULL;
>      }
>  
> -    if ((item = ring_get_tail(&dev->write_bufs_pool))) {
> +    if ((item = ring_get_tail(&dev->priv->write_bufs_pool))) {
>          ret = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
>          ring_remove(item);
> -        dev->cur_pool_size -= ret->buf_size;
> +        dev->priv->cur_pool_size -= ret->buf_size;
>      } else {
>          ret = spice_new0(SpiceCharDeviceWriteBuffer, 1);
>      }
> @@ -637,15 +662,15 @@ static SpiceCharDeviceWriteBuffer
> *__spice_char_device_write_buffer_get(
>              goto error;
>          }
>      } else if (origin == WRITE_BUFFER_ORIGIN_SERVER) {
> -        dev->num_self_tokens--;
> +        dev->priv->num_self_tokens--;
>      }
>  
>      ret->token_price = migrated_data_tokens ? migrated_data_tokens : 1;
>      ret->refs = 1;
>      return ret;
>  error:
> -    dev->cur_pool_size += ret->buf_size;
> -    ring_add(&dev->write_bufs_pool, &ret->link);
> +    dev->priv->cur_pool_size += ret->buf_size;
> +    ring_add(&dev->priv->write_bufs_pool, &ret->link);
>      return NULL;
>  }
>  
> @@ -694,7 +719,7 @@ void
> spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
>          return;
>      }
>  
> -    ring_add(&dev->write_queue, &write_buf->link);
> +    ring_add(&dev->priv->write_queue, &write_buf->link);
>      spice_char_device_write_to_device(dev);
>  }
>  
> @@ -712,7 +737,7 @@ void
> spice_char_device_write_buffer_release(SpiceCharDeviceState *dev,
>          return;
>      }
>  
> -    spice_assert(dev->cur_write_buf != write_buf);
> +    spice_assert(dev->priv->cur_write_buf != write_buf);
>  
>      spice_char_device_write_buffer_pool_add(dev, write_buf);
>      if (buf_origin == WRITE_BUFFER_ORIGIN_CLIENT) {
> @@ -724,7 +749,7 @@ void
> spice_char_device_write_buffer_release(SpiceCharDeviceState *dev,
>          spice_assert(dev_client);
>          spice_char_device_client_tokens_add(dev, dev_client,
>          buf_token_price);
>      } else if (buf_origin == WRITE_BUFFER_ORIGIN_SERVER) {
> -        dev->num_self_tokens++;
> +        dev->priv->num_self_tokens++;
>          spice_char_device_on_free_self_token(dev);
>      }
>  }
> @@ -740,96 +765,27 @@ SpiceCharDeviceState
> *spice_char_device_state_create(SpiceCharDeviceInstance *si
>                                                       SpiceCharDeviceCallbacks
>                                                       *cbs,
>                                                       void *opaque)
>  {
> -    SpiceCharDeviceState *char_dev;
> -    SpiceCharDeviceInterface *sif;
> -
> -    spice_assert(sin);
> -    spice_assert(cbs->read_one_msg_from_device && cbs->ref_msg_to_client &&
> -                 cbs->unref_msg_to_client && cbs->send_msg_to_client &&
> -                 cbs->send_tokens_to_client && cbs->remove_client);
> -
> -    char_dev = spice_new0(SpiceCharDeviceState, 1);
> -    char_dev->sin = sin;
> -    char_dev->reds = reds;
> -    char_dev->cbs = *cbs;
> -    char_dev->opaque = opaque;
> -    char_dev->client_tokens_interval = client_tokens_interval;
> -    char_dev->num_self_tokens = self_tokens;
> -
> -    ring_init(&char_dev->write_queue);
> -    ring_init(&char_dev->write_bufs_pool);
> -    ring_init(&char_dev->clients);
> -
> -    sif = spice_char_device_get_interface(char_dev->sin);
> -    if (sif->base.minor_version <= 2 ||
> -        !(sif->flags & SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
> -        char_dev->write_to_dev_timer = reds_core_timer_add(reds,
> spice_char_dev_write_retry, char_dev);
> -        if (!char_dev->write_to_dev_timer) {
> -            spice_error("failed creating char dev write timer");
> -        }
> -    }
> -
> -    char_dev->refs = 1;
> -    sin->st = char_dev;
> -    spice_debug("sin %p dev_state %p", sin, char_dev);
> -    return char_dev;
> +    return red_char_device_new(sin, reds, client_tokens_interval,
> +                               self_tokens, cbs, opaque);
>  }
>  
>  void spice_char_device_state_reset_dev_instance(SpiceCharDeviceState *state,
>                                                  SpiceCharDeviceInstance
>                                                  *sin)
>  {
>      spice_debug("sin %p dev_state %p", sin, state);
> -    state->sin = sin;
> +    state->priv->sin = sin;
>      sin->st = state;
>  }
>  
>  void *spice_char_device_state_opaque_get(SpiceCharDeviceState *dev)
>  {
> -    return dev->opaque;
> -}
> -
> -static void spice_char_device_state_ref(SpiceCharDeviceState *char_dev)
> -{
> -    char_dev->refs++;
> -}
> -
> -static void spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
> -{
> -    /* The refs field protects the char_dev from being deallocated in
> -     * case spice_char_device_state_destroy has been called
> -     * during a callabck, and we might still access the char_dev afterwards.
> -     * spice_char_device_state_unref is always coupled with a preceding
> -     * spice_char_device_state_ref. Here, refs can turn 0
> -     * only when spice_char_device_state_destroy is called in between
> -     * the calls to spice_char_device_state_ref and
> spice_char_device_state_unref.*/
> -    if (!--char_dev->refs) {
> -        free(char_dev);
> -    }
> +    return dev->priv->opaque;
>  }
>  
>  void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
>  {
> -    reds_on_char_device_state_destroy(char_dev->reds, char_dev);
> -    if (char_dev->write_to_dev_timer) {
> -        reds_core_timer_remove(char_dev->reds,
> char_dev->write_to_dev_timer);
> -        char_dev->write_to_dev_timer = NULL;
> -    }
> -    write_buffers_queue_free(&char_dev->write_queue);
> -    write_buffers_queue_free(&char_dev->write_bufs_pool);
> -    char_dev->cur_pool_size = 0;
> -    spice_char_device_write_buffer_free(char_dev->cur_write_buf);
> -    char_dev->cur_write_buf = NULL;
> -
> -    while (!ring_is_empty(&char_dev->clients)) {
> -        RingItem *item = ring_get_tail(&char_dev->clients);
> -        SpiceCharDeviceClientState *dev_client;
> -
> -        dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState,
> link);
> -        spice_char_device_client_free(char_dev, dev_client);
> -    }
> -    char_dev->running = FALSE;
> -
> -    spice_char_device_state_unref(char_dev);
> +    g_return_if_fail(RED_IS_CHAR_DEVICE(char_dev));
> +    g_object_unref(char_dev);
>  }
>  
>  static SpiceCharDeviceClientState *red_char_device_client_new(RedClient
>  *client,
> @@ -840,6 +796,7 @@ static SpiceCharDeviceClientState
> *red_char_device_client_new(RedClient *client,
>  {
>      SpiceCharDeviceClientState *dev_client;
>  
> +    /* Dev client creation */
>      dev_client = spice_new0(SpiceCharDeviceClientState, 1);
>      dev_client->client = client;
>      ring_init(&dev_client->send_queue);
> @@ -859,6 +816,7 @@ static SpiceCharDeviceClientState
> *red_char_device_client_new(RedClient *client,
>          dev_client->num_client_tokens = ~0;
>          dev_client->num_send_tokens = ~0;
>      }
> +    /* end of dev client creation */
>  
>      return dev_client;
>  }
> @@ -876,13 +834,13 @@ int spice_char_device_client_add(SpiceCharDeviceState
> *dev,
>      spice_assert(dev);
>      spice_assert(client);
>  
> -    if (wait_for_migrate_data && (dev->num_clients > 0 || dev->active)) {
> +    if (wait_for_migrate_data && (dev->priv->num_clients > 0 ||
> dev->priv->active)) {
>          spice_warning("can't restore device %p from migration data. The
>          device "
>                        "has already been active", dev);
>          return FALSE;
>      }
>  
> -    dev->wait_for_migrate_data = wait_for_migrate_data;
> +    dev->priv->wait_for_migrate_data = wait_for_migrate_data;
>  
>      spice_debug("dev_state %p client %p", dev, client);
>      dev_client = red_char_device_client_new(client, do_flow_control,
> @@ -890,8 +848,8 @@ int spice_char_device_client_add(SpiceCharDeviceState
> *dev,
>                                              num_client_tokens,
>                                              num_send_tokens);
>      dev_client->dev = dev;
> -    ring_add(&dev->clients, &dev_client->link);
> -    dev->num_clients++;
> +    ring_add(&dev->priv->clients, &dev_client->link);
> +    dev->priv->num_clients++;
>      /* Now that we have a client, forward any pending device data */
>      spice_char_device_wakeup(dev);
>      return TRUE;
> @@ -910,16 +868,16 @@ void
> spice_char_device_client_remove(SpiceCharDeviceState *dev,
>          return;
>      }
>      spice_char_device_client_free(dev, dev_client);
> -    if (dev->wait_for_migrate_data) {
> -        spice_assert(dev->num_clients == 0);
> -        dev->wait_for_migrate_data  = FALSE;
> +    if (dev->priv->wait_for_migrate_data) {
> +        spice_assert(dev->priv->num_clients == 0);
> +        dev->priv->wait_for_migrate_data  = FALSE;
>          spice_char_device_read_from_device(dev);
>      }
>  
> -    if (dev->num_clients == 0) {
> -        spice_debug("client removed, memory pool will be freed (%"PRIu64"
> bytes)", dev->cur_pool_size);
> -        write_buffers_queue_free(&dev->write_bufs_pool);
> -        dev->cur_pool_size = 0;
> +    if (dev->priv->num_clients == 0) {
> +        spice_debug("client removed, memory pool will be freed (%"PRIu64"
> bytes)", dev->priv->cur_pool_size);
> +        write_buffers_queue_free(&dev->priv->write_bufs_pool);
> +        dev->priv->cur_pool_size = 0;
>      }
>  }
>  
> @@ -932,7 +890,7 @@ int spice_char_device_client_exists(SpiceCharDeviceState
> *dev,
>  void spice_char_device_start(SpiceCharDeviceState *dev)
>  {
>      spice_debug("dev_state %p", dev);
> -    dev->running = TRUE;
> +    dev->priv->running = TRUE;
>      spice_char_device_state_ref(dev);
>      while (spice_char_device_write_to_device(dev) ||
>             spice_char_device_read_from_device(dev));
> @@ -942,10 +900,10 @@ void spice_char_device_start(SpiceCharDeviceState *dev)
>  void spice_char_device_stop(SpiceCharDeviceState *dev)
>  {
>      spice_debug("dev_state %p", dev);
> -    dev->running = FALSE;
> -    dev->active = FALSE;
> -    if (dev->write_to_dev_timer) {
> -        reds_core_timer_cancel(dev->reds, dev->write_to_dev_timer);
> +    dev->priv->running = FALSE;
> +    dev->priv->active = FALSE;
> +    if (dev->priv->write_to_dev_timer) {
> +        reds_core_timer_cancel(dev->priv->reds,
> dev->priv->write_to_dev_timer);
>      }
>  }
>  
> @@ -954,10 +912,10 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
>      RingItem *client_item;
>  
>      spice_char_device_stop(dev);
> -    dev->wait_for_migrate_data = FALSE;
> +    dev->priv->wait_for_migrate_data = FALSE;
>      spice_debug("dev_state %p", dev);
> -    while (!ring_is_empty(&dev->write_queue)) {
> -        RingItem *item = ring_get_tail(&dev->write_queue);
> +    while (!ring_is_empty(&dev->priv->write_queue)) {
> +        RingItem *item = ring_get_tail(&dev->priv->write_queue);
>          SpiceCharDeviceWriteBuffer *buf;
>  
>          ring_remove(item);
> @@ -965,20 +923,20 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
>          /* tracking the tokens */
>          spice_char_device_write_buffer_release(dev, buf);
>      }
> -    if (dev->cur_write_buf) {
> -        SpiceCharDeviceWriteBuffer *release_buf = dev->cur_write_buf;
> +    if (dev->priv->cur_write_buf) {
> +        SpiceCharDeviceWriteBuffer *release_buf = dev->priv->cur_write_buf;
>  
> -        dev->cur_write_buf = NULL;
> +        dev->priv->cur_write_buf = NULL;
>          spice_char_device_write_buffer_release(dev, release_buf);
>      }
>  
> -    RING_FOREACH(client_item, &dev->clients) {
> +    RING_FOREACH(client_item, &dev->priv->clients) {
>          SpiceCharDeviceClientState *dev_client;
>  
>          dev_client = SPICE_CONTAINEROF(client_item,
>          SpiceCharDeviceClientState, link);
>          spice_char_device_client_send_queue_free(dev, dev_client);
>      }
> -    dev->sin = NULL;
> +    dev->priv->sin = NULL;
>  }
>  
>  void spice_char_device_wakeup(SpiceCharDeviceState *dev)
> @@ -1020,8 +978,8 @@ void
> spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
>      SpiceMarshaller *m2;
>  
>      /* multi-clients are not supported */
> -    spice_assert(dev->num_clients == 1);
> -    client_state = SPICE_CONTAINEROF(ring_get_tail(&dev->clients),
> +    spice_assert(dev->priv->num_clients == 1);
> +    client_state = SPICE_CONTAINEROF(ring_get_tail(&dev->priv->clients),
>                                       SpiceCharDeviceClientState,
>                                       link);
>      /* FIXME: if there were more than one client before the marshalling,
> @@ -1038,21 +996,21 @@ void
> spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
>      *write_to_dev_tokens_ptr = 0;
>  
>      m2 = spice_marshaller_get_ptr_submarshaller(m, 0);
> -    if (dev->cur_write_buf) {
> -        uint32_t buf_remaining = dev->cur_write_buf->buf +
> dev->cur_write_buf->buf_used -
> -                                 dev->cur_write_buf_pos;
> -        spice_marshaller_add_ref_full(m2, dev->cur_write_buf_pos,
> buf_remaining,
> +    if (dev->priv->cur_write_buf) {
> +        uint32_t buf_remaining = dev->priv->cur_write_buf->buf +
> dev->priv->cur_write_buf->buf_used -
> +                                 dev->priv->cur_write_buf_pos;
> +        spice_marshaller_add_ref_full(m2, dev->priv->cur_write_buf_pos,
> buf_remaining,
>                                        migrate_data_marshaller_write_buffer_free,
> -
> spice_char_device_write_buffer_ref(dev->cur_write_buf)
> +
> spice_char_device_write_buffer_ref(dev->priv->cur_write_buf)
>                                        );
>          *write_to_dev_size_ptr += buf_remaining;
> -        if (dev->cur_write_buf->origin == WRITE_BUFFER_ORIGIN_CLIENT) {
> -            spice_assert(dev->cur_write_buf->client ==
> client_state->client);
> -            (*write_to_dev_tokens_ptr) += dev->cur_write_buf->token_price;
> +        if (dev->priv->cur_write_buf->origin == WRITE_BUFFER_ORIGIN_CLIENT)
> {
> +            spice_assert(dev->priv->cur_write_buf->client ==
> client_state->client);
> +            (*write_to_dev_tokens_ptr) +=
> dev->priv->cur_write_buf->token_price;
>          }
>      }
>  
> -    RING_FOREACH_REVERSED(item, &dev->write_queue) {
> +    RING_FOREACH_REVERSED(item, &dev->priv->write_queue) {
>          SpiceCharDeviceWriteBuffer *write_buf;
>  
>          write_buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer,
>          link);
> @@ -1076,9 +1034,9 @@ int
> spice_char_device_state_restore(SpiceCharDeviceState *dev,
>      SpiceCharDeviceClientState *client_state;
>      uint32_t client_tokens_window;
>  
> -    spice_assert(dev->num_clients == 1 && dev->wait_for_migrate_data);
> +    spice_assert(dev->priv->num_clients == 1 &&
> dev->priv->wait_for_migrate_data);
>  
> -    client_state = SPICE_CONTAINEROF(ring_get_tail(&dev->clients),
> +    client_state = SPICE_CONTAINEROF(ring_get_tail(&dev->priv->clients),
>                                       SpiceCharDeviceClientState,
>                                       link);
>      if (mig_data->version > SPICE_MIGRATE_DATA_CHAR_DEVICE_VERSION) {
> @@ -1086,7 +1044,7 @@ int
> spice_char_device_state_restore(SpiceCharDeviceState *dev,
>                      dev, mig_data->version,
>                      SPICE_MIGRATE_DATA_CHAR_DEVICE_VERSION);
>          return FALSE;
>      }
> -    spice_assert(!dev->cur_write_buf && ring_is_empty(&dev->write_queue));
> +    spice_assert(!dev->priv->cur_write_buf &&
> ring_is_empty(&dev->priv->write_queue));
>      spice_assert(mig_data->connected);
>  
>      client_tokens_window = client_state->num_client_tokens; /* initial state
>      of tokens */
> @@ -1099,23 +1057,23 @@ int
> spice_char_device_state_restore(SpiceCharDeviceState *dev,
>  
>      if (mig_data->write_size > 0) {
>          if (mig_data->write_num_client_tokens) {
> -            dev->cur_write_buf =
> +            dev->priv->cur_write_buf =
>                  __spice_char_device_write_buffer_get(dev,
>                  client_state->client,
>                      mig_data->write_size, WRITE_BUFFER_ORIGIN_CLIENT,
>                      mig_data->write_num_client_tokens);
>          } else {
> -            dev->cur_write_buf =
> +            dev->priv->cur_write_buf =
>                  __spice_char_device_write_buffer_get(dev, NULL,
>                      mig_data->write_size, WRITE_BUFFER_ORIGIN_SERVER, 0);
>          }
>          /* the first write buffer contains all the data that was saved for
>          migration */
> -        memcpy(dev->cur_write_buf->buf,
> +        memcpy(dev->priv->cur_write_buf->buf,
>                 ((uint8_t *)mig_data) + mig_data->write_data_ptr -
>                 sizeof(SpiceMigrateDataHeader),
>                 mig_data->write_size);
> -        dev->cur_write_buf->buf_used = mig_data->write_size;
> -        dev->cur_write_buf_pos = dev->cur_write_buf->buf;
> +        dev->priv->cur_write_buf->buf_used = mig_data->write_size;
> +        dev->priv->cur_write_buf_pos = dev->priv->cur_write_buf->buf;
>      }
> -    dev->wait_for_migrate_data = FALSE;
> +    dev->priv->wait_for_migrate_data = FALSE;
>      spice_char_device_write_to_device(dev);
>      spice_char_device_read_from_device(dev);
>      return TRUE;
> @@ -1123,10 +1081,238 @@ int
> spice_char_device_state_restore(SpiceCharDeviceState *dev,
>  
>  SpiceServer* spice_char_device_get_server(SpiceCharDeviceState *dev)
>  {
> -    return dev->reds;
> +    return dev->priv->reds;
>  }
>  
>  SpiceCharDeviceInterface
>  *spice_char_device_get_interface(SpiceCharDeviceInstance *instance)
>  {
>     return SPICE_CONTAINEROF(instance->base.sif, SpiceCharDeviceInterface,
>     base);
>  }
> +
> +
> +static void red_char_device_init_device_instance(RedCharDevice *self)
> +{
> +    SpiceCharDeviceInterface *sif;
> +
> +    g_return_if_fail(self->priv->reds);
> +
> +    if (self->priv->write_to_dev_timer) {
> +        reds_core_timer_remove(self->priv->reds,
> self->priv->write_to_dev_timer);
> +        self->priv->write_to_dev_timer = NULL;
> +    }
> +    if (self->priv->sin == NULL) {
> +       return;
> +    }
> +
> +    sif = spice_char_device_get_interface(self->priv->sin);
> +    if (sif->base.minor_version <= 2 ||
> +        !(sif->flags & SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
> +        self->priv->write_to_dev_timer =
> reds_core_timer_add(self->priv->reds,
> +
> spice_char_dev_write_retry,
> +                                                             self);
> +        if (!self->priv->write_to_dev_timer) {
> +            spice_error("failed creating char dev write timer");
> +        }
> +    }
> +
> +    self->priv->sin->st = self;
> +}
> +
> +static void
> +red_char_device_get_property(GObject    *object,
> +                             guint       property_id,
> +                             GValue     *value,
> +                             GParamSpec *pspec)
> +{
> +    RedCharDevice *self = RED_CHAR_DEVICE(object);
> +
> +    switch (property_id)
> +    {
> +        case PROP_CHAR_DEV_INSTANCE:
> +            g_value_set_pointer(value, self->priv->sin);
> +            break;
> +        case PROP_SPICE_SERVER:
> +            g_value_set_pointer(value, self->priv->reds);
> +            break;
> +        case PROP_CLIENT_TOKENS_INTERVAL:
> +            g_value_set_uint64(value, self->priv->client_tokens_interval);
> +            break;
> +        case PROP_SELF_TOKENS:
> +            g_value_set_uint64(value, self->priv->num_self_tokens);
> +            break;
> +        case PROP_OPAQUE:
> +            g_value_set_pointer(value, self->priv->opaque);
> +            break;
> +        default:
> +            G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> +    }
> +}
> +
> +static void
> +red_char_device_set_property(GObject      *object,
> +                             guint         property_id,
> +                             const GValue *value,
> +                             GParamSpec   *pspec)
> +{
> +    RedCharDevice *self = RED_CHAR_DEVICE(object);
> +
> +    switch (property_id)
> +    {
> +        case PROP_CHAR_DEV_INSTANCE:
> +            self->priv->sin = g_value_get_pointer(value);
> +            break;
> +        case PROP_SPICE_SERVER:
> +            self->priv->reds = g_value_get_pointer(value);
> +            break;
> +        case PROP_CLIENT_TOKENS_INTERVAL:
> +            self->priv->client_tokens_interval = g_value_get_uint64(value);
> +            break;
> +        case PROP_SELF_TOKENS:
> +            self->priv->num_self_tokens = g_value_get_uint64(value);
> +            break;
> +        case PROP_OPAQUE:
> +            self->priv->opaque = g_value_get_pointer(value);
> +            break;
> +        default:
> +            G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> +    }
> +}
> +
> +static void
> +red_char_device_on_sin_changed(GObject *object,
> +                               GParamSpec *pspec G_GNUC_UNUSED,
> +                               gpointer user_data G_GNUC_UNUSED)
> +{
> +    RedCharDevice *self = RED_CHAR_DEVICE(object);
> +
> +    red_char_device_init_device_instance(self);
> +}
> +
> +static void
> +red_char_device_finalize(GObject *object)
> +{
> +    RedCharDevice *self = RED_CHAR_DEVICE(object);
> +
> +    /* FIXME: replace with g_object_weak_ref () */
> +    reds_on_char_device_state_destroy(self->priv->reds, self);
> +    self->priv->cur_pool_size = 0;
> +    reds_core_timer_remove(self->priv->reds,
> self->priv->write_to_dev_timer);
> +    write_buffers_queue_free(&self->priv->write_queue);
> +    write_buffers_queue_free(&self->priv->write_bufs_pool);
> +    if (self->priv->cur_write_buf) {
> +        spice_char_device_write_buffer_free(self->priv->cur_write_buf);
> +    }
> +
> +    while (!ring_is_empty(&self->priv->clients)) {
> +        RingItem *item = ring_get_tail(&self->priv->clients);
> +        SpiceCharDeviceClientState *dev_client;
> +
> +        dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState,
> link);
> +        spice_char_device_client_free(self, dev_client);
> +    }
> +    self->priv->running = FALSE;
> +
> +    G_OBJECT_CLASS(red_char_device_parent_class)->finalize(object);
> +}
> +

This finalize is a bit different from the original destroy (order and some
if). Could be that just this patch was based on old code and all rebases
make the two function differs. Christophe.. do you remember any specific
reason why this new finalize and old destroy are different?

> +static void
> +red_char_device_class_init(RedCharDeviceClass *klass)
> +{
> +    GObjectClass *object_class = G_OBJECT_CLASS(klass);
> +
> +    g_type_class_add_private(klass, sizeof (RedCharDevicePrivate));
> +
> +    object_class->get_property = red_char_device_get_property;
> +    object_class->set_property = red_char_device_set_property;
> +    object_class->finalize = red_char_device_finalize;
> +
> +    g_object_class_install_property(object_class,
> +                                    PROP_CHAR_DEV_INSTANCE,
> +                                    g_param_spec_pointer("sin",
> +                                                         "Char device
> instance",
> +                                                         "Char device
> instance",
> +
> G_PARAM_STATIC_STRINGS
> |
> +                                                         G_PARAM_READWRITE |
> +
> G_PARAM_CONSTRUCT));
> +    g_object_class_install_property(object_class,
> +                                    PROP_SPICE_SERVER,
> +                                    g_param_spec_pointer("spice-server",
> +                                                         "RedsState
> instance",

In many other patches the nick as the same value as the name.

> +                                                         "RedsState
> instance",
> +
> G_PARAM_STATIC_STRINGS
> |
> +
> G_PARAM_READWRITE));
> +    g_object_class_install_property(object_class,
> +                                    PROP_CLIENT_TOKENS_INTERVAL,
> +
> g_param_spec_uint64("client-tokens-interval",
> +                                                        "Client token
> interval",
> +                                                        "Client token
> interval",
> +                                                        0, G_MAXUINT64, 0,
> +
> G_PARAM_STATIC_STRINGS
> |
> +                                                        G_PARAM_READWRITE));
> +    g_object_class_install_property(object_class,
> +                                    PROP_SELF_TOKENS,
> +                                    g_param_spec_uint64("self-tokens",
> +                                                        "Self tokens",
> +                                                        "Self tokens",
> +                                                        0, G_MAXUINT64, 0,
> +
> G_PARAM_STATIC_STRINGS
> |
> +                                                        G_PARAM_READWRITE));
> +    g_object_class_install_property(object_class,
> +                                    PROP_OPAQUE,
> +                                    g_param_spec_pointer("opaque",
> +                                                         "opaque",
> +                                                         "User data to pass
> to callbacks",
> +                                                      G_PARAM_STATIC_STRINGS
> |
> +                                                      G_PARAM_READWRITE));
> +
> +}
> +
> +static void
> +red_char_device_init(RedCharDevice *self)
> +{
> +    self->priv = RED_CHAR_DEVICE_PRIVATE(self);
> +
> +    ring_init(&self->priv->write_queue);
> +    ring_init(&self->priv->write_bufs_pool);
> +    ring_init(&self->priv->clients);
> +
> +    g_signal_connect(self, "notify::sin",
> G_CALLBACK(red_char_device_on_sin_changed), NULL);
> +}
> +
> +static RedCharDevice *
> +red_char_device_new(SpiceCharDeviceInstance *sin,
> +                    RedsState *reds,
> +                    uint32_t client_tokens_interval,
> +                    uint32_t self_tokens,
> +                    SpiceCharDeviceCallbacks *cbs,
> +                    void *opaque)
> +{
> +    RedCharDevice *char_dev;
> +
> +    char_dev = g_object_new(RED_TYPE_CHAR_DEVICE,
> +                            "sin", sin,
> +                            "reds", reds,
> +                            "client-tokens-interval",
> client_tokens_interval,
> +                            "self-tokens", self_tokens,
> +                            "opaque", opaque,
> +                            NULL);
> +

One stuff I don't like about some GObject functions are the really bad
type unsafety. This lines are going to crash quite badly on 32 bit machines
as interval and some other properties are going to be passed as 32 bit but read
as 64 causing code to lose the sync and interpreting one field as another.
In this case properly casts to guint64 solve the issue.

> +    /* FIXME: redundant with the "opaque" property in g_object_new */
> +    red_char_device_set_callbacks(char_dev, cbs, opaque);
> +

I think the solution would be add a new "cbs" properties and pass to g_object_new.
Or I would change the FIXME to a TODO.

> +    return char_dev;
> +}
> +
> +/* FIXME: needs to be moved to class vfuncs once all child classes are
> gobjects */

I would use a TODO here

> +void
> +red_char_device_set_callbacks(RedCharDevice *dev,
> +                              SpiceCharDeviceCallbacks *cbs,
> +                              gpointer opaque)

Why not using a static function?

> +{
> +    g_assert(cbs->read_one_msg_from_device && cbs->ref_msg_to_client &&
> +             cbs->unref_msg_to_client && cbs->send_msg_to_client &&
> +             cbs->send_tokens_to_client && cbs->remove_client);
> +
> +    dev->priv->cbs = *cbs;
> +    dev->priv->opaque = opaque;
> +}
> diff --git a/server/char-device.h b/server/char-device.h
> index 7c78524..9a1eb95 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -18,10 +18,44 @@
>  #ifndef CHAR_DEVICE_H_
>  #define CHAR_DEVICE_H_
>  
> +#include <glib-object.h>
> +

As GObject is going to be quite common why not putting the include
in red-common.h ?

>  #include "spice.h"
>  #include "red-channel.h"
>  #include "migration-protocol.h"
>  
> +#define RED_TYPE_CHAR_DEVICE red_char_device_get_type()
> +
> +#define RED_CHAR_DEVICE(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> RED_TYPE_CHAR_DEVICE, RedCharDevice))
> +#define RED_CHAR_DEVICE_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST((klass),
> RED_TYPE_CHAR_DEVICE, RedCharDeviceClass))
> +#define RED_IS_CHAR_DEVICE(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj),
> RED_TYPE_CHAR_DEVICE))
> +#define RED_IS_CHAR_DEVICE_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass),
> RED_TYPE_CHAR_DEVICE))
> +#define RED_CHAR_DEVICE_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS((obj),
> RED_TYPE_CHAR_DEVICE, RedCharDeviceClass))
> +
> +typedef struct SpiceCharDeviceState RedCharDevice;
> +typedef struct RedCharDeviceClass RedCharDeviceClass;
> +typedef struct RedCharDevicePrivate RedCharDevicePrivate;
> +
> +/* 'SpiceCharDeviceState' name is used for consistency with what
> spice-char.h exports */
> +struct SpiceCharDeviceState
> +{
> +    GObject parent;
> +
> +    RedCharDevicePrivate *priv;
> +};
> +
> +struct RedCharDeviceClass
> +{
> +    GObjectClass parent_class;
> +};
> +
> +GType red_char_device_get_type(void) G_GNUC_CONST;
> +
> +typedef struct SpiceCharDeviceCallbacks SpiceCharDeviceCallbacks;
> +void red_char_device_set_callbacks(RedCharDevice *dev,
> +                                   SpiceCharDeviceCallbacks *cbs,
> +                                   gpointer opaque);
> +
>  /*
>   * Shared code for char devices, mainly for flow control.
>   *
> @@ -97,7 +131,7 @@ typedef struct SpiceCharDeviceWriteBuffer {
>  
>  typedef void SpiceCharDeviceMsgToClient;
>  
> -typedef struct SpiceCharDeviceCallbacks {
> +struct SpiceCharDeviceCallbacks {
>      /*
>       * Messages that are addressed to the client can be queued in case we
>       have
>       * multiple clients and some of them don't have enough tokens.
> @@ -127,7 +161,7 @@ typedef struct SpiceCharDeviceCallbacks {
>       * due to slow flow or due to some other error.
>       * The called instance should disconnect the client, or at least the
>       corresponding channel */
>      void (*remove_client)(RedClient *client, void *opaque);
> -} SpiceCharDeviceCallbacks;
> +};
>  
>  SpiceCharDeviceState *spice_char_device_state_create(SpiceCharDeviceInstance
>  *sin,
>                                                       struct RedsState *reds,

Frediano


More information about the Spice-devel mailing list