Re: [PATCH 14/20] Use GQueue for RedCharDevice::send_queue

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2016-04-14 at 16:50 -0500, Jonathon Jongsma wrote:
> From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> 
> There was an extra RedCharDeviceMsgToClientItem type whose only
> purpose was to manage a linked list of items to send. GQueue has the
> same purpose as this type in addition to being generic. As the length of
> the send queue is tracked, a GQueue is more appropriate than a GList and
> allow to remove RedCharDevice::send_queue_size.
> ---
>  server/char-device.c | 69 ++++++++++++++++++---------------------------------
> -
>  1 file changed, 23 insertions(+), 46 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index f3e16da..6b9596e 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -41,8 +41,7 @@ struct RedCharDeviceClient {
>      uint64_t num_send_tokens; /* send to client */
>      SpiceTimer *wait_for_tokens_timer;
>      int wait_for_tokens_started;
> -    Ring send_queue;
> -    uint32_t send_queue_size;
> +    GQueue *send_queue;
>      uint32_t max_send_queue_size;
>  };
>  
> @@ -105,11 +104,6 @@ static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
>  static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
> *write_buf);
>  static void red_char_device_write_retry(void *opaque);
>  
> -typedef struct RedCharDeviceMsgToClientItem {
> -    RingItem link;
> -    PipeItem *msg;
> -} RedCharDeviceMsgToClientItem;
> -
>  static PipeItem *
>  red_char_device_read_one_msg_from_device(RedCharDevice *dev)
>  {
> @@ -187,19 +181,10 @@ static void
> red_char_device_write_buffer_pool_add(RedCharDevice *dev,
>  static void red_char_device_client_send_queue_free(RedCharDevice *dev,
>                                                     RedCharDeviceClient
> *dev_client)
>  {
> -    spice_debug("send_queue_empty %d", ring_is_empty(&dev_client
> ->send_queue));
> -    while (!ring_is_empty(&dev_client->send_queue)) {
> -        RingItem *item = ring_get_tail(&dev_client->send_queue);
> -        RedCharDeviceMsgToClientItem *msg_item = SPICE_CONTAINEROF(item,
> -                                                                  
>  RedCharDeviceMsgToClientItem,
> -                                                                   link);
> -
> -        ring_remove(item);
> -        pipe_item_unref(msg_item->msg);
> -        free(msg_item);
> -    }
> -    dev_client->num_send_tokens += dev_client->send_queue_size;
> -    dev_client->send_queue_size = 0;
> +    spice_debug("send_queue_empty %d", g_queue_is_empty(dev_client
> ->send_queue));
> +    g_queue_free_full(dev_client->send_queue, pipe_item_unref);
> +    g_queue_clear(dev_client->send_queue);
> +    dev_client->num_send_tokens += g_queue_get_length(dev_client
> ->send_queue);

This looks wrong. We're calling g_queue_get_length() immediately after clearing
the queue, so it's guaranteed to be 0. This last line should be moved before the
g_queue_free_full() call.

>  }
>  
>  static void red_char_device_client_free(RedCharDevice *dev,
> @@ -303,17 +288,14 @@ static void
> red_char_device_add_msg_to_client_queue(RedCharDeviceClient *dev_cli
>                                                      PipeItem *msg)
>  {
>      RedCharDevice *dev = dev_client->dev;
> -    RedCharDeviceMsgToClientItem *msg_item;
>  
> -    if (dev_client->send_queue_size >= dev_client->max_send_queue_size) {
> +    if (g_queue_get_length(dev_client->send_queue) >= dev_client
> ->max_send_queue_size) {
>          red_char_device_handle_client_overflow(dev_client);
>          return;
>      }
>  
> -    msg_item = spice_new0(RedCharDeviceMsgToClientItem, 1);
> -    msg_item->msg = pipe_item_ref(msg);
> -    ring_add(&dev_client->send_queue, &msg_item->link);
> -    dev_client->send_queue_size++;
> +    pipe_item_ref(msg);
> +    g_queue_push_head(dev_client->send_queue, msg);
>      if (!dev_client->wait_for_tokens_started) {
>          reds_core_timer_start(dev->priv->reds, dev_client
> ->wait_for_tokens_timer,
>                                RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
> @@ -332,7 +314,7 @@ static void
> red_char_device_send_msg_to_clients(RedCharDevice *dev,
>          dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient, link);
>          if (red_char_device_can_send_to_client(dev_client)) {
>              dev_client->num_send_tokens--;
> -            spice_assert(ring_is_empty(&dev_client->send_queue));
> +            spice_assert(g_queue_is_empty(dev_client->send_queue));
>              red_char_device_send_msg_to_client(dev, msg, dev_client->client);
>  
>              /* don't refer to dev_client anymore, it may have been released
> */
> @@ -394,21 +376,17 @@ static int
> red_char_device_read_from_device(RedCharDevice *dev)
>  
>  static void red_char_device_client_send_queue_push(RedCharDeviceClient
> *dev_client)
>  {
> -    RingItem *item;
> -    while ((item = ring_get_tail(&dev_client->send_queue)) &&
> -           red_char_device_can_send_to_client(dev_client)) {
> -        RedCharDeviceMsgToClientItem *msg_item;
> -
> -        msg_item = SPICE_CONTAINEROF(item, RedCharDeviceMsgToClientItem,
> link);
> -        ring_remove(item);
> -
> +    while (!g_queue_is_empty(dev_client->send_queue)) {
> +        GList *msg;

GList* ?? Doesn't this GQueue contain PipeItem*? 

> +        if (!red_char_device_can_send_to_client(dev_client)) {
> +            break;
> +        }
> +        msg = g_queue_pop_tail(dev_client->send_queue);
> +        g_assert(msg != NULL);
>          dev_client->num_send_tokens--;
> -        red_char_device_send_msg_to_client(dev_client->dev,
> -                                           msg_item->msg,
> +        red_char_device_send_msg_to_client(dev_client->dev, msg->data,

I think the second argument should simply be 'msg' instead of 'msg->data', no? I
wonder why it works...

>                                             dev_client->client);
> -        pipe_item_unref(msg_item->msg);
> -        dev_client->send_queue_size--;
> -        free(msg_item);
> +        pipe_item_unref(msg);


...and here, we free this GList* variable by calling pipe_item_unref(). That's
one of the pitfalls of making pipe_item_unref() accept a gpointer argument --
the compiler didn't warn us...

>      }
>  }
>  
> @@ -418,7 +396,7 @@ static void
> red_char_device_send_to_client_tokens_absorb(RedCharDeviceClient *de
>      RedCharDevice *dev = dev_client->dev;
>      dev_client->num_send_tokens += tokens;
>  
> -    if (dev_client->send_queue_size) {
> +    if (g_queue_get_length(dev_client->send_queue)) {
>          spice_assert(dev_client->num_send_tokens == tokens);
>          red_char_device_client_send_queue_push(dev_client);
>      }
> @@ -427,7 +405,7 @@ static void
> red_char_device_send_to_client_tokens_absorb(RedCharDeviceClient *de
>          reds_core_timer_cancel(dev->priv->reds, dev_client
> ->wait_for_tokens_timer);
>          dev_client->wait_for_tokens_started = FALSE;
>          red_char_device_read_from_device(dev_client->dev);
> -    } else if (dev_client->send_queue_size) {
> +    } else if (!g_queue_is_empty(dev_client->send_queue)) {
>          reds_core_timer_start(dev->priv->reds, dev_client
> ->wait_for_tokens_timer,
>                                RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
>          dev_client->wait_for_tokens_started = TRUE;
> @@ -751,8 +729,7 @@ static RedCharDeviceClient
> *red_char_device_client_new(RedClient *client,
>  
>      dev_client = spice_new0(RedCharDeviceClient, 1);
>      dev_client->client = client;
> -    ring_init(&dev_client->send_queue);
> -    dev_client->send_queue_size = 0;
> +    dev_client->send_queue = g_queue_new();
>      dev_client->max_send_queue_size = max_send_queue_size;
>      dev_client->do_flow_control = do_flow_control;
>      if (do_flow_control) {
> @@ -934,9 +911,9 @@ void red_char_device_migrate_data_marshall(RedCharDevice
> *dev,
>                                     RedCharDeviceClient,
>                                     link);
>      /* FIXME: if there were more than one client before the marshalling,
> -     * it is possible that the send_queue_size > 0, and the send data
> +     * it is possible that the send_queue length > 0, and the send data
>       * should be migrated as well */
> -    spice_assert(dev_client->send_queue_size == 0);
> +    spice_assert(g_queue_is_empty(dev_client->send_queue));
>      spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_CHAR_DEVICE_VERSION);
>      spice_marshaller_add_uint8(m, 1); /* connected */
>      spice_marshaller_add_uint32(m, dev_client->num_client_tokens);
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]