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

[Frediano Ziglio]

> 
> On Fri, Nov 20, 2015 at 12:17 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >
> > ---
> >  server/cursor-channel.c |  4 ++++
> >  server/red_worker.c     | 60
> >  +++++++++++++++++++++----------------------------
> >  2 files changed, 30 insertions(+), 34 deletions(-)
> >
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index aafc807..5eb2647 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -484,6 +484,10 @@ CursorChannelClient*
> > cursor_channel_client_new(CursorChannel *cursor, RedClient
> >      ring_init(&ccc->cursor_cache_lru);
> >      ccc->cursor_cache_available = CLIENT_CURSOR_CACHE_SIZE;
> >
> > +    RedChannelClient *rcc = RED_CHANNEL_CLIENT(ccc);
> > +    red_channel_client_ack_zero_messages_window(rcc);
> > +    red_channel_client_push_set_ack(rcc);
> > +
> >      return ccc;
> >  }
> >

Style opinion: a _new function should just initialize the object,
not also initialize a connection sending messages.

I don't like this part of the patch

> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index b8dfbb9..377eb36 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -5420,34 +5420,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 surface_dirty_region_to_rects(RedSurface *surface,
> >                                            QXLRect *qxl_dirty_rects,
> >                                            uint32_t num_dirty_rects)
> > @@ -5955,19 +5927,39 @@ 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);
> > +
> > +    // 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);
> 
> It's not exactly related to this patch, but do we already have an
> answer for this TODO? :-)
> 

Usually in the code canvas is checked to check surface validity.
Surfaces are allocated in a static array where you can have used and not used.
I don't understand why the check is not done using reference counting.

> > +}
> > +
> >  /* 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);
> >  }
> > --
> > 2.4.3
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> ACK!
> --
> Fabiano Fidêncio
> 

I'll try to post an update.

Frediano