[Spice-devel] [PATCH 06/18] worker: tidy up cursor_connect a bit

[Jonathon Jongsma]

On Mon, 2015-11-23 at 17:01 +0000, Frediano Ziglio wrote:
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> ---
>  server/red_worker.c | 65 +++++++++++++++++++++++++---------------------------
> -
>  1 file changed, 31 insertions(+), 34 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index f64befa..e0fd6e5 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -4889,34 +4889,6 @@ static void handle_new_display_channel(RedWorker
> *worker, RedClient *client, Red
>      dcc_start(dcc);
>  }
>  
> -static void red_connect_cursor(RedWorker *worker, RedClient *client,
> RedsStream *stream,
> -                               int migrate,
> -                               uint32_t *common_caps, int num_common_caps,
> -                               uint32_t *caps, int num_caps)
> -{
> -    CursorChannel *channel;
> -    CursorChannelClient *ccc;
> -
> -    spice_return_if_fail(worker->cursor_channel != NULL);
> -
> -    channel = worker->cursor_channel;
> -    spice_info("add cursor channel client");
> -    ccc = cursor_channel_client_new(channel, client, stream,
> -                                    migrate,
> -                                    common_caps, num_common_caps,
> -                                    caps, num_caps);
> -    spice_return_if_fail(ccc != NULL);
> -
> -    RedChannelClient *rcc = RED_CHANNEL_CLIENT(ccc);
> -    red_channel_client_ack_zero_messages_window(rcc);
> -    red_channel_client_push_set_ack(rcc);
> -
> -    // TODO: why do we check for context.canvas? defer this to after display
> cc is connected
> -    // and test it's canvas? this is just a test to see if there is an active
> renderer?
> -    if (display_channel_surface_has_canvas(worker->display_channel, 0))
> -        cursor_channel_init(channel, ccc);
> -}
> -
>  static void region_to_qxlrects(QRegion *region, QXLRect *qxl_rects, uint32_t
> num_rects)
>  {
>      SpiceRect *rects;
> @@ -5422,19 +5394,44 @@ static void handle_dev_monitors_config_async(void
> *opaque, void *payload)
>      red_worker_push_monitors_config(worker);
>  }
>  
> +static void cursor_connect(RedWorker *worker, RedClient *client, RedsStream
> *stream,
> +                           int migrate,
> +                           uint32_t *common_caps, int num_common_caps,
> +                           uint32_t *caps, int num_caps)
> +{
> +    CursorChannel *channel = worker->cursor_channel;
> +    CursorChannelClient *ccc;
> +
> +    spice_return_if_fail(worker->cursor_channel != NULL);
> +
> +    spice_info("add cursor channel client");
> +    ccc = cursor_channel_client_new(channel, client, stream,
> +                                    migrate,
> +                                    common_caps, num_common_caps,
> +                                    caps, num_caps);
> +    spice_return_if_fail(ccc != NULL);
> +
> +    RedChannelClient *rcc = RED_CHANNEL_CLIENT(ccc);
> +    red_channel_client_ack_zero_messages_window(rcc);
> +    red_channel_client_push_set_ack(rcc);
> +
> +    // TODO: why do we check for context.canvas? defer this to after display
> cc is connected
> +    // and test it's canvas? this is just a test to see if there is an active
> renderer?
> +    if (display_channel_surface_has_canvas(worker->display_channel, 0))
> +        cursor_channel_init(channel, ccc);
> +}
> +
>  /* TODO: special, perhaps use another dispatcher? */
>  static void handle_dev_cursor_connect(void *opaque, void *payload)
>  {
>      RedWorkerMessageCursorConnect *msg = payload;
>      RedWorker *worker = opaque;
> -    RedsStream *stream = msg->stream;
> -    RedClient *client = msg->client;
> -    int migration = msg->migration;
>  
>      spice_info("cursor connect");
> -    red_connect_cursor(worker, client, stream, migration,
> -                       msg->common_caps, msg->num_common_caps,
> -                       msg->caps, msg->num_caps);
> +    cursor_connect(worker,
> +                   msg->client, msg->stream, msg->migration,
> +                   msg->common_caps, msg->num_common_caps,
> +                   msg->caps, msg->num_caps);
>      free(msg->caps);
>      free(msg->common_caps);
>  }


I don't really see much point to most of this commit. But I don't see much
reason *not* to do it either...  Basically all it does is:

- renames the function
- moves the definition to a different place in the source file (not sure why)
- avoids creating local variables in the caller.

So I'm pretty ambivalent. But I'll ACK it to avoid creating conflicts further in
the branch.

Jonathon