[Spice-devel] [PATCH 10/10] Change RedCharDevicePrivate::clients to GList
Frediano Ziglio
fziglio at redhat.com
Fri Sep 9 14:47:12 UTC 2016
>
> More Ring cleanup
Personally I don't approve the rationale.
Rings are used by Qemu and Linux kernel, Qemu calls us directly and
we deal with Linux too. Not counting all BSD code. So surely are not
less tested and maintained then glib code.
Rings are not less readable or less type safe then GList or any
glib container (personally I think that ring_is_empty is more
readable than comparing a container with NULL).
Rings are more space and computation efficient.
Rings require however to embed container knowledge in the item
structures which not desirable in many situations.
And expanding manually macros is IMHO not more readable nor
more maintainable.
> ---
> server/char-device.c | 72
> ++++++++++++++++++++++------------------------------
> 1 file changed, 30 insertions(+), 42 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index e01f34c..40512f8 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;
> @@ -200,8 +198,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);
> }
>
> @@ -215,12 +212,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;
> }
> @@ -246,13 +242,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;
> @@ -288,12 +282,13 @@ 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 *item, *next;
>
> - RING_FOREACH_SAFE(item, next, &dev->priv->clients) {
> - RedCharDeviceClient *dev_client;
> + item = dev->priv->clients;
> + while (item) {
> + next = item->next;
> + RedCharDeviceClient *dev_client = item->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));
> @@ -303,6 +298,7 @@ static void
> red_char_device_send_msg_to_clients(RedCharDevice *dev,
> } else {
> red_char_device_add_msg_to_client_queue(dev_client, msg);
> }
> + item = next;
> }
> }
>
> @@ -331,7 +327,7 @@ static int red_char_device_read_from_device(RedCharDevice
> *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->priv->clients)) &&
> dev->priv->running) {
> + while ((max_send_tokens || (dev->priv->clients == NULL)) &&
> dev->priv->running) {
> RedPipeItem *msg;
>
> msg = red_char_device_read_one_msg_from_device(dev);
> @@ -741,7 +737,7 @@ int red_char_device_client_add(RedCharDevice *dev,
> spice_assert(dev);
> spice_assert(client);
>
> - if (wait_for_migrate_data && (dev->priv->num_clients > 0 ||
> dev->priv->active)) {
> + if (wait_for_migrate_data && (dev->priv->clients != NULL ||
> dev->priv->active)) {
> spice_warning("can't restore device %p from migration data. The
> device "
> "has already been active", dev);
> return FALSE;
> @@ -755,8 +751,7 @@ int red_char_device_client_add(RedCharDevice *dev,
> num_client_tokens,
> num_send_tokens);
> dev_client->dev = dev;
> - ring_add(&dev->priv->clients, &dev_client->link);
> - dev->priv->num_clients++;
> + dev->priv->clients = g_list_prepend(dev->priv->clients, dev_client);
> /* Now that we have a client, forward any pending device data */
> red_char_device_wakeup(dev);
> return TRUE;
> @@ -776,12 +771,12 @@ void red_char_device_client_remove(RedCharDevice *dev,
> }
> red_char_device_client_free(dev, dev_client);
> if (dev->priv->wait_for_migrate_data) {
> - spice_assert(dev->priv->num_clients == 0);
> + spice_assert(dev->priv->clients == NULL);
> dev->priv->wait_for_migrate_data = FALSE;
> red_char_device_read_from_device(dev);
> }
>
> - if (dev->priv->num_clients == 0) {
> + if (dev->priv->clients == NULL) {
> spice_debug("client removed, memory pool will be freed (%"PRIu64"
> bytes)", dev->priv->cur_pool_size);
> g_list_free_full(dev->priv->write_bufs_pool,
> (GDestroyNotify)
> red_char_device_write_buffer_free);
> @@ -818,7 +813,7 @@ void red_char_device_stop(RedCharDevice *dev)
>
> void red_char_device_reset(RedCharDevice *dev)
> {
> - RingItem *client_item;
> + GList *client_item;
>
> red_char_device_stop(dev);
> dev->priv->wait_for_migrate_data = FALSE;
> @@ -828,10 +823,9 @@ void red_char_device_reset(RedCharDevice *dev)
> dev->priv->write_queue = NULL;
> red_char_device_write_buffer_release(dev, &dev->priv->cur_write_buf);
>
> - RING_FOREACH(client_item, &dev->priv->clients) {
> - RedCharDeviceClient *dev_client;
> + for (client_item = dev->priv->clients; client_item != NULL; client_item
> = client_item->next) {
> + RedCharDeviceClient *dev_client = client_item->data;
>
> - dev_client = SPICE_CONTAINEROF(client_item, RedCharDeviceClient,
> link);
> 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)red_pipe_item_unref,
> NULL);
> @@ -885,10 +879,8 @@ void red_char_device_migrate_data_marshall(RedCharDevice
> *dev,
> SpiceMarshaller *m2;
>
> /* multi-clients are not supported */
> - spice_assert(dev->priv->num_clients == 1);
> - dev_client = SPICE_CONTAINEROF(ring_get_tail(&dev->priv->clients),
> - RedCharDeviceClient,
> - link);
> + spice_assert(g_list_length(dev->priv->clients) == 1);
> + dev_client = g_list_last(dev->priv->clients)->data;
> /* FIXME: if there were more than one client before the marshalling,
> * it is possible that the send_queue length > 0, and the send data
> * should be migrated as well */
> @@ -940,11 +932,10 @@ int red_char_device_restore(RedCharDevice *dev,
> RedCharDeviceClient *dev_client;
> uint32_t client_tokens_window;
>
> - spice_assert(dev->priv->num_clients == 1 &&
> dev->priv->wait_for_migrate_data);
> + spice_assert(g_list_length(dev->priv->clients) == 1 &&
> + dev->priv->wait_for_migrate_data);
>
> - dev_client = SPICE_CONTAINEROF(ring_get_tail(&dev->priv->clients),
> - RedCharDeviceClient,
> - link);
> + dev_client = g_list_last(dev->priv->clients)->data;
> if (mig_data->version > SPICE_MIGRATE_DATA_CHAR_DEVICE_VERSION) {
> spice_error("dev %p error: migration data version %u is bigger than
> self %u",
> dev, mig_data->version,
> SPICE_MIGRATE_DATA_CHAR_DEVICE_VERSION);
> @@ -1113,11 +1104,10 @@ red_char_device_finalize(GObject *object)
> red_char_device_write_buffer_free(self->priv->cur_write_buf);
> self->priv->cur_write_buf = NULL;
>
> - while (!ring_is_empty(&self->priv->clients)) {
> - RingItem *item = ring_get_tail(&self->priv->clients);
> - RedCharDeviceClient *dev_client;
> + while (self->priv->clients != NULL) {
> + GList *item = g_list_last(self->priv->clients);
> + RedCharDeviceClient *dev_client = item->data;
>
> - dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient, link);
> red_char_device_client_free(self, dev_client);
> }
> self->priv->running = FALSE;
> @@ -1183,7 +1173,5 @@ red_char_device_init(RedCharDevice *self)
> {
> self->priv = RED_CHAR_DEVICE_PRIVATE(self);
>
> - ring_init(&self->priv->clients);
> -
> g_signal_connect(self, "notify::sin",
> G_CALLBACK(red_char_device_on_sin_changed), NULL);
> }
These lists are small (actually we support only a single client!)
Acked-by: Frediano Ziglio <fziglio at redhat.com>
Frediano
More information about the Spice-devel
mailing list