[Spice-devel] [PATCH spice-server] red-channel: Store client callback data in ClientCbs

Frediano Ziglio fziglio at redhat.com
Wed Jun 27 07:13:00 UTC 2018


> 
> Hey,
> 
> This loops back on that old/unresolved discussion
> https://lists.freedesktop.org/archives/spice-devel/2017-August/039541.html
> I'd rather not make changes like this for now as we don't know where we
> want to go with these ClientCbs ;)
> 

Yes, I know, was mainly a pending follow up.

> The only reason why we have these ClientCbs is to be able to marshall
> calls from the channel thread to the main thread if needed, and to
> decide whether to marshall or not, we only need to know if the current
> channel needs a dispatcher or not, and call the corresponding channel
> specific method either directly, or ensure the call gets to the correct
> thread.
> 

Agreed

> ClientCbs is definitely not something which needs to be registered
> per-channel instance, it's more of a class thing, and the dispatcher is
> more something which could be thread local. I would not stick everything
> together like this before cleaning up more of that mess...
> 

Disagreed, is not a class thing, it's should be a responsibility of however
assign the thread to the specific instance.
Is not much thread local, more "instance local" I would say.

> Christophe
> 
> On Fri, Jun 22, 2018 at 09:24:09AM +0100, Frediano Ziglio wrote:
> > Instead of using a separate parameter and field store
> > in the same structure.
> > This allows to pass the pointer while passing callbacks
> > and also make easier to understand that "data" field
> > is related to callbacks.
> > Also allows to retrieve client callback saved pointer.
> > Remove the g_object_set_data using the data registered.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-channel.c |  6 ++++++
> >  server/red-channel.h |  3 +++
> >  server/red-qxl.c     | 14 ++++++++------
> >  server/red-worker.c  |  2 --
> >  4 files changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index 448b690e..1167fd79 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -374,6 +374,12 @@ void red_channel_register_client_cbs(RedChannel
> > *channel, const ClientCbs *clien
> >      if (client_cbs->migrate) {
> >          channel->priv->client_cbs.migrate = client_cbs->migrate;
> >      }
> > +    channel->priv->client_cbs.data = client_cbs->data;
> > +}
> > +
> > +void *red_channel_get_client_cbs_data(RedChannel *channel)
> > +{
> > +    return channel->priv->client_cbs.data;
> >  }
> >  
> >  static void add_capability(uint32_t **caps, int *num_caps, uint32_t cap)
> > diff --git a/server/red-channel.h b/server/red-channel.h
> > index 4e7a9066..8d68eeb0 100644
> > --- a/server/red-channel.h
> > +++ b/server/red-channel.h
> > @@ -71,6 +71,7 @@ typedef struct {
> >      channel_client_connect_proc connect;
> >      channel_client_disconnect_proc disconnect;
> >      channel_client_migrate_proc migrate;
> > +    void *data;
> >  } ClientCbs;
> >  
> >  static inline gboolean test_capability(const uint32_t *caps, int num_caps,
> >  uint32_t cap)
> > @@ -123,6 +124,8 @@ void red_channel_remove_client(RedChannel *channel,
> > RedChannelClient *rcc);
> >  void red_channel_init_stat_node(RedChannel *channel, const RedStatNode
> >  *parent, const char *name);
> >  
> >  void red_channel_register_client_cbs(RedChannel *channel, const ClientCbs
> >  *client_cbs);
> > +// retrieve data pointer stored with red_channel_register_client_cbs
> > +void *red_channel_get_client_cbs_data(RedChannel *channel);
> >  // caps are freed when the channel is destroyed
> >  void red_channel_set_common_cap(RedChannel *channel, uint32_t cap);
> >  void red_channel_set_cap(RedChannel *channel, uint32_t cap);
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index 730b9f3d..4b36acdb 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -78,7 +78,7 @@ static void red_qxl_set_display_peer(RedChannel *channel,
> > RedClient *client,
> >      Dispatcher *dispatcher;
> >  
> >      spice_debug("%s", "");
> > -    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > "dispatcher");
> > +    dispatcher = (Dispatcher *)red_channel_get_client_cbs_data(channel);
> >      // get a reference potentially the main channel can be destroyed in
> >      // the main thread causing RedClient to be destroyed before using it
> >      payload.client = g_object_ref(client);
> > @@ -97,7 +97,7 @@ static void
> > red_qxl_disconnect_display_peer(RedChannelClient *rcc)
> >      Dispatcher *dispatcher;
> >      RedChannel *channel = red_channel_client_get_channel(rcc);
> >  
> > -    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > "dispatcher");
> > +    dispatcher = (Dispatcher *)red_channel_get_client_cbs_data(channel);
> >  
> >      spice_printerr("");
> >      payload.rcc = rcc;
> > @@ -117,7 +117,7 @@ static void red_qxl_display_migrate(RedChannelClient
> > *rcc)
> >  
> >      red_channel_printerr(channel, "");
> >  
> > -    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > "dispatcher");
> > +    dispatcher = (Dispatcher *)red_channel_get_client_cbs_data(channel);
> >      payload.rcc = g_object_ref(rcc);
> >      dispatcher_send_message(dispatcher,
> >                              RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> > @@ -129,7 +129,7 @@ static void red_qxl_set_cursor_peer(RedChannel
> > *channel, RedClient *client, RedS
> >                                      RedChannelCapabilities *caps)
> >  {
> >      RedWorkerMessageCursorConnect payload = {0,};
> > -    Dispatcher *dispatcher = (Dispatcher
> > *)g_object_get_data(G_OBJECT(channel), "dispatcher");
> > +    Dispatcher *dispatcher = (Dispatcher
> > *)red_channel_get_client_cbs_data(channel);
> >      spice_printerr("");
> >      // get a reference potentially the main channel can be destroyed in
> >      // the main thread causing RedClient to be destroyed before using it
> > @@ -149,7 +149,7 @@ static void
> > red_qxl_disconnect_cursor_peer(RedChannelClient *rcc)
> >      Dispatcher *dispatcher;
> >      RedChannel *channel = red_channel_client_get_channel(rcc);
> >  
> > -    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > "dispatcher");
> > +    dispatcher = (Dispatcher *)red_channel_get_client_cbs_data(channel);
> >      spice_printerr("");
> >      payload.rcc = rcc;
> >  
> > @@ -166,7 +166,7 @@ static void red_qxl_cursor_migrate(RedChannelClient
> > *rcc)
> >  
> >      red_channel_printerr(channel, "");
> >  
> > -    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > "dispatcher");
> > +    dispatcher = (Dispatcher *)red_channel_get_client_cbs_data(channel);
> >      payload.rcc = g_object_ref(rcc);
> >      dispatcher_send_message(dispatcher,
> >                              RED_WORKER_MESSAGE_CURSOR_MIGRATE,
> > @@ -917,10 +917,12 @@ void red_qxl_init(RedsState *reds, QXLInstance *qxl)
> >      client_cursor_cbs.connect = red_qxl_set_cursor_peer;
> >      client_cursor_cbs.disconnect = red_qxl_disconnect_cursor_peer;
> >      client_cursor_cbs.migrate = red_qxl_cursor_migrate;
> > +    client_cursor_cbs.data = qxl_state->dispatcher;
> >  
> >      client_display_cbs.connect = red_qxl_set_display_peer;
> >      client_display_cbs.disconnect = red_qxl_disconnect_display_peer;
> >      client_display_cbs.migrate = red_qxl_display_migrate;
> > +    client_display_cbs.data = qxl_state->dispatcher;
> >  
> >      qxl_state->worker = red_worker_new(qxl, &client_cursor_cbs,
> >                                         &client_display_cbs);
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index 21ae6d77..fa3697a3 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -1338,7 +1338,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> >      channel = RED_CHANNEL(worker->cursor_channel);
> >      red_channel_init_stat_node(channel, &worker->stat, "cursor_channel");
> >      red_channel_register_client_cbs(channel, client_cursor_cbs);
> > -    g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
> >      reds_register_channel(reds, channel);
> >  
> >      // TODO: handle seemless migration. Temp, setting migrate to FALSE
> > @@ -1349,7 +1348,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> >      channel = RED_CHANNEL(worker->display_channel);
> >      red_channel_init_stat_node(channel, &worker->stat, "display_channel");
> >      red_channel_register_client_cbs(channel, client_display_cbs);
> > -    g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
> >      reds_register_channel(reds, channel);
> >  
> >      return worker;


More information about the Spice-devel mailing list