[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