[Spice-devel] [PATCH 04/14] Replace RedChannel::clients with GList

[Christophe Fergeau]

Hey,

This patch mostly looks good to me, a few minor comments:

On Tue, May 03, 2016 at 03:00:20PM -0500, Jonathon Jongsma wrote:
> Instead of using a Ring, use a GList to store the list of channel
> clients. This allows us to iterate the clients without poking inside of
> the client struct to get the channel_link. This is required in order to
> make the RedChannelClient struct private.
> ---
>  server/display-channel.c |  64 ++++++++--------
>  server/display-channel.h |  16 ++--
>  server/main-channel.c    |  42 +++++------
>  server/red-channel.c     | 193 ++++++++++++++++++++---------------------------
>  server/red-channel.h     |   3 +-
>  server/red-worker.c      |   8 +-
>  server/red-worker.h      |  15 ++--
>  server/stream.c          |  23 +++---
>  8 files changed, 163 insertions(+), 201 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 1f4d66f..8c040d7 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1200,9 +1198,9 @@ int display_channel_wait_for_migrate_data(DisplayChannel *display)
>      }
>  
>      spice_debug(NULL);
> -    spice_warn_if_fail(channel->clients_num == 1);
> +    spice_warn_if_fail(g_list_length(channel->clients) == 1);
>  
> -    rcc = SPICE_CONTAINEROF(ring_get_head(&channel->clients), RedChannelClient, channel_link);
> +    rcc = channel->clients->data;

There are various places in the patch which access directly
clients->data, sometimes we assert first that there is a single item in
that list, sometimes note. I'd use g_list_nth_data(clients, 0); rather
than directly accessing 'clients', this way it's more explicit that we
are only dealing with one item in the list.

> diff --git a/server/red-worker.c b/server/red-worker.c
> index f21fefb..5b6bc7b 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -501,8 +501,8 @@ static void guest_set_client_capabilities(RedWorker *worker)
>      int i;
>      DisplayChannelClient *dcc;
>      RedChannelClient *rcc;
> -    RingItem *link, *next;
> -    uint8_t caps[SPICE_CAPABILITIES_SIZE] = { 0 };
> +    GList *link, *next;
> +    uint8_t caps[58] = { 0 }; /* FIXME: 58?? */

This looks like the 'caps' line shouldn't have been modified.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160510/a04ee2e2/attachment.sig>