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

[Frediano Ziglio]

> 
> 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
> 

In the previous version the cursor_channel_client_new was also sending some
initial message so cursor_connect was shorter. However I moved these lines
back.
Perhaps I should add me as signed-off and change the commit message.
Wait... why the patch is just moving a function from one place to another
in the same file.. is confusing and also the function will be moved to a
different file later... I'll change it.

NACK for now.

Frediano