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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel