[Spice-devel] [PATCH spice-server] red-channel: Store client callback data in ClientCbs
Christophe Fergeau
cfergeau at redhat.com
Tue Jun 26 12:14:57 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 ;)
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.
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...
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;
> --
> 2.17.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180626/897f7422/attachment.sig>
More information about the Spice-devel
mailing list