[Spice-devel] [PATCH 04/14] Use GQueue for RedCharDevice::send_queue
Jonathon Jongsma
jjongsma at redhat.com
Wed Apr 27 14:34:19 UTC 2016
Anybody object to me acking this patch despite the fact that I modified it and
posted a new version?
I'll merge it soon if there are no objections.
On Thu, 2016-04-21 at 16:43 -0500, Jonathon Jongsma wrote:
> From: Christophe Fergeau <cfergeau at redhat.com>
>
> 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.
> ---
> Fixed an invalid memory access introduced in the previous patch due to calling
> g_queue_clear() after g_queue_free_full()
>
> server/char-device.c | 72 +++++++++++++++------------------------------------
> -
> 1 file changed, 20 insertions(+), 52 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index b258b8b..a8d31e8 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)
> {
> @@ -184,24 +178,6 @@ static void
> red_char_device_write_buffer_pool_add(RedCharDevice *dev,
> red_char_device_write_buffer_unref(buf);
> }
>
> -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;
> -}
> -
> static void red_char_device_client_free(RedCharDevice *dev,
> RedCharDeviceClient *dev_client)
> {
> @@ -212,7 +188,7 @@ static void red_char_device_client_free(RedCharDevice
> *dev,
> dev_client->wait_for_tokens_timer = NULL;
> }
>
> - red_char_device_client_send_queue_free(dev, dev_client);
> + g_queue_free_full(dev_client->send_queue, pipe_item_unref);
>
> /* remove write buffers that are associated with the client */
> spice_debug("write_queue_is_empty %d", ring_is_empty(&dev->priv
> ->write_queue) && !dev->priv->cur_write_buf);
> @@ -303,17 +279,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 +305,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 +367,14 @@ 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)) &&
> + while (!g_queue_is_empty(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);
> -
> + PipeItem *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,
> dev_client->client);
> - pipe_item_unref(msg_item->msg);
> - dev_client->send_queue_size--;
> - free(msg_item);
> + pipe_item_unref(msg);
> }
> }
>
> @@ -418,7 +384,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 +393,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;
> @@ -753,8 +719,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) {
> @@ -887,7 +852,10 @@ void red_char_device_reset(RedCharDevice *dev)
> RedCharDeviceClient *dev_client;
>
> dev_client = SPICE_CONTAINEROF(client_item, RedCharDeviceClient,
> link);
> - red_char_device_client_send_queue_free(dev, dev_client);
> + spice_debug("send_queue_empty %d", g_queue_is_empty(dev_client
> ->send_queue));
> + dev_client->num_send_tokens += g_queue_get_length(dev_client
> ->send_queue);
> + g_queue_foreach(dev_client->send_queue, (GFunc)pipe_item_unref,
> NULL);
> + g_queue_clear(dev_client->send_queue);
> }
> red_char_device_reset_dev_instance(dev, NULL);
> }
> @@ -936,9 +904,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);
More information about the Spice-devel
mailing list