[Spice-devel] [PATCH spice-server 2/2] Make channel client callbacks virtual functions
Frediano Ziglio
fziglio at redhat.com
Wed Mar 20 09:53:34 UTC 2019
>
> Rather than having an API to register client callbacks for each channel
> type, make them vfuncs.
>
> Since the client callbacks are registered identically for each channel
> of the same type, it doesn't make sense for to require these to be
> registered separately for each object. It's cleaner to have these be
> per-class properties, so they've been converted to virtual functions.
>
Why they are vfunc of RedChannel but accepts RedChannelClient?
I think this is just a bad inheritance where everything was in a single
file/class.
> Note that In order to avoid dependencies from the display and cursor
> channels to the worker (necessary to perform the message dispatching
> required), the worker itself subclasses these channels to provide
> dispatching client callbacks.
> ---
> server/cursor-channel.c | 4 +
> server/display-channel.c | 29 ----
> server/display-channel.h | 7 -
> server/inputs-channel.c | 9 +-
> server/main-channel.c | 7 +-
> server/red-channel.c | 35 ++---
> server/red-channel.h | 19 ++-
> server/red-qxl.c | 111 +-------------
> server/red-stream-device.c | 5 -
> server/red-worker.c | 280 ++++++++++++++++++++++++++++++++++--
> server/red-worker.h | 4 +-
> server/smartcard.c | 6 +-
> server/sound.c | 18 ++-
> server/spicevmc.c | 7 +-
> server/stream-channel.c | 5 +-
> server/tests/test-channel.c | 13 +-
> 16 files changed, 316 insertions(+), 243 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index d751211af..749c1c6b8 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -383,6 +383,10 @@ cursor_channel_class_init(CursorChannelClass *klass)
> channel_class->handle_message = red_channel_client_handle_message;
>
> channel_class->send_item = cursor_channel_send_item;
> +
> + // client callbacks
> + channel_class->connect = (channel_client_connect_proc)
> cursor_channel_connect;
> + channel_class->migrate = cursor_channel_client_migrate;
> }
>
> static void
> diff --git a/server/display-channel.c b/server/display-channel.c
> index e179abfd3..71912c1b9 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2224,35 +2224,6 @@ static SpiceCanvas
> *image_surfaces_get(SpiceImageSurfaces *surfaces, uint32_t su
> return p->surfaces[surface_id].context.canvas;
> }
>
> -DisplayChannel* display_channel_new(RedsState *reds,
> - QXLInstance *qxl,
> - const SpiceCoreInterfaceInternal *core,
> - int migrate, int stream_video,
> - GArray *video_codecs,
> - uint32_t n_surfaces)
> -{
> - DisplayChannel *display;
> -
> - /* FIXME: migrate is not used...? */
> - spice_debug("create display channel");
> - display = g_object_new(TYPE_DISPLAY_CHANNEL,
> - "spice-server", reds,
> - "core-interface", core,
> - "channel-type", SPICE_CHANNEL_DISPLAY,
> - "id", qxl->id,
> - "migration-flags",
> - (SPICE_MIGRATE_NEED_FLUSH |
> SPICE_MIGRATE_NEED_DATA_TRANSFER),
> - "qxl", qxl,
> - "n-surfaces", n_surfaces,
> - "video-codecs", video_codecs,
> - "handle-acks", TRUE,
> - NULL);
> - if (display) {
> - display_channel_set_stream_video(display, stream_video);
> - }
> - return display;
> -}
> -
> static SpiceCanvas *image_surfaces_get(SpiceImageSurfaces *surfaces,
> uint32_t surface_id);
> static void drawables_init(DisplayChannel *display);
> static void
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 948018cf3..83727e17a 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -96,13 +96,6 @@ struct Drawable {
> DisplayChannel *display;
> };
>
> -DisplayChannel* display_channel_new
> (RedsState *reds,
> -
> QXLInstance
> *qxl,
> - const
> SpiceCoreInterfaceInternal *core,
> - int
> migrate,
> - int
> stream_video,
> - GArray
> *video_codecs,
> -
> uint32_t
> n_surfaces);
> void display_channel_create_surface
> (DisplayChannel *display, uint32_t surface_id,
> uint32_t
> width,
> uint32_t
> height,
> int32_t
> stride,
> uint32_t
> format,
> void
> *line_0,
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index a7df62e2a..eff5421ae 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -542,17 +542,12 @@ InputsChannel* inputs_channel_new(RedsState *reds)
> static void
> inputs_channel_constructed(GObject *object)
> {
> - ClientCbs client_cbs = { NULL, };
> InputsChannel *self = INPUTS_CHANNEL(object);
> RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> SpiceCoreInterfaceInternal *core =
> red_channel_get_core_interface(RED_CHANNEL(self));
>
> G_OBJECT_CLASS(inputs_channel_parent_class)->constructed(object);
>
> - client_cbs.connect = inputs_connect;
> - client_cbs.migrate = inputs_migrate;
> - red_channel_register_client_cbs(RED_CHANNEL(self), &client_cbs);
> -
> red_channel_set_cap(RED_CHANNEL(self), SPICE_INPUTS_CAP_KEY_SCANCODE);
> reds_register_channel(reds, RED_CHANNEL(self));
>
> @@ -596,6 +591,10 @@ inputs_channel_class_init(InputsChannelClass *klass)
> channel_class->send_item = inputs_channel_send_item;
> channel_class->handle_migrate_data = inputs_channel_handle_migrate_data;
> channel_class->handle_migrate_flush_mark =
> inputs_channel_handle_migrate_flush_mark;
> +
> + // client callbacks
> + channel_class->connect = inputs_connect;
> + channel_class->migrate = inputs_migrate;
> }
>
> static SpiceKbdInstance* inputs_channel_get_keyboard(InputsChannel *inputs)
> diff --git a/server/main-channel.c b/server/main-channel.c
> index f866fb4ae..a48e74be1 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -264,15 +264,11 @@ static void
> main_channel_constructed(GObject *object)
> {
> MainChannel *self = MAIN_CHANNEL(object);
> - ClientCbs client_cbs = { NULL, };
>
> G_OBJECT_CLASS(main_channel_parent_class)->constructed(object);
>
> red_channel_set_cap(RED_CHANNEL(self),
> SPICE_MAIN_CAP_SEMI_SEAMLESS_MIGRATE);
> red_channel_set_cap(RED_CHANNEL(self), SPICE_MAIN_CAP_SEAMLESS_MIGRATE);
> -
> - client_cbs.migrate = main_channel_client_migrate;
> - red_channel_register_client_cbs(RED_CHANNEL(self), &client_cbs);
> }
>
> static void
> @@ -295,6 +291,9 @@ main_channel_class_init(MainChannelClass *klass)
> channel_class->send_item = main_channel_client_send_item;
> channel_class->handle_migrate_flush_mark =
> main_channel_handle_migrate_flush_mark;
> channel_class->handle_migrate_data = main_channel_handle_migrate_data;
> +
> + // client callbacks
> + channel_class->migrate = main_channel_client_migrate;
> }
>
> static int main_channel_connect_semi_seamless(MainChannel *main_channel)
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 1d88739ec..9cbb5cf55 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -86,7 +86,6 @@ struct RedChannelPrivate
> RedChannelCapabilities local_caps;
> uint32_t migration_flags;
>
> - ClientCbs client_cbs;
> // TODO: when different channel_clients are in different threads
> // from Channel -> need to protect!
> pthread_t thread_id;
> @@ -279,6 +278,10 @@ red_channel_class_init(RedChannelClass *klass)
> G_PARAM_CONSTRUCT_ONLY |
> G_PARAM_STATIC_STRINGS);
> g_object_class_install_property(object_class, PROP_MIGRATION_FLAGS,
> spec);
> +
> + klass->connect = red_channel_client_default_connect;
> + klass->disconnect = red_channel_client_default_disconnect;
> + klass->migrate = red_channel_client_default_migrate;
> }
>
> static void
> @@ -288,10 +291,6 @@ red_channel_init(RedChannel *self)
>
> red_channel_set_common_cap(self, SPICE_COMMON_CAP_MINI_HEADER);
> self->priv->thread_id = pthread_self();
> -
> - self->priv->client_cbs.connect = red_channel_client_default_connect;
> - self->priv->client_cbs.disconnect =
> red_channel_client_default_disconnect;
> - self->priv->client_cbs.migrate = red_channel_client_default_migrate;
> }
>
> // utility to avoid possible invalid function cast
> @@ -357,20 +356,6 @@ const RedStatNode *red_channel_get_stat_node(RedChannel
> *channel)
> return &channel->priv->stat;
> }
>
> -void red_channel_register_client_cbs(RedChannel *channel, const ClientCbs
> *client_cbs)
> -{
> - spice_assert(client_cbs->connect || channel->priv->type ==
> SPICE_CHANNEL_MAIN);
> - channel->priv->client_cbs.connect = client_cbs->connect;
> -
> - if (client_cbs->disconnect) {
> - channel->priv->client_cbs.disconnect = client_cbs->disconnect;
> - }
> -
> - if (client_cbs->migrate) {
> - channel->priv->client_cbs.migrate = client_cbs->migrate;
> - }
> -}
> -
> static void add_capability(uint32_t **caps, int *num_caps, uint32_t cap)
> {
> int nbefore, n;
> @@ -487,7 +472,9 @@ void red_channel_connect(RedChannel *channel, RedClient
> *client,
> RedStream *stream, int migration,
> RedChannelCapabilities *caps)
> {
> - channel->priv->client_cbs.connect(channel, client, stream, migration,
> caps);
> + RedChannelClass *klass = RED_CHANNEL_GET_CLASS(channel);
> + g_return_if_fail(klass->connect != NULL);
> + klass->connect(channel, client, stream, migration, caps);
> }
>
> GList *red_channel_get_clients(RedChannel *channel)
> @@ -692,10 +679,14 @@ const RedChannelCapabilities*
> red_channel_get_local_capabilities(RedChannel *sel
>
> void red_channel_migrate_client(RedChannel *channel, RedChannelClient *rcc)
> {
> - channel->priv->client_cbs.migrate(rcc);
> + RedChannelClass *klass = RED_CHANNEL_GET_CLASS(channel);
> + g_return_if_fail(klass->migrate != NULL);
> + klass->migrate(rcc);
> }
>
> void red_channel_disconnect_client(RedChannel *channel, RedChannelClient
> *rcc)
> {
> - channel->priv->client_cbs.disconnect(rcc);
> + RedChannelClass *klass = RED_CHANNEL_GET_CLASS(channel);
> + g_return_if_fail(klass->disconnect != NULL);
> + klass->disconnect(rcc);
> }
> diff --git a/server/red-channel.h b/server/red-channel.h
> index bb3a95e8b..4bfd81ee1 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -63,16 +63,6 @@ typedef void
> (*channel_client_disconnect_proc)(RedChannelClient *base);
> typedef void (*channel_client_migrate_proc)(RedChannelClient *base);
>
>
> -/*
> - * callbacks that are triggered from client events.
> - * They should be called from the thread that handles the RedClient
> - */
> -typedef struct {
> - channel_client_connect_proc connect;
> - channel_client_disconnect_proc disconnect;
> - channel_client_migrate_proc migrate;
> -} ClientCbs;
> -
> static inline gboolean test_capability(const uint32_t *caps, int num_caps,
> uint32_t cap)
> {
> return VD_AGENT_HAS_CAPABILITY(caps, num_caps, cap);
> @@ -107,6 +97,14 @@ struct RedChannelClass
> channel_handle_migrate_flush_mark_proc handle_migrate_flush_mark;
> channel_handle_migrate_data_proc handle_migrate_data;
> channel_handle_migrate_data_get_serial_proc
> handle_migrate_data_get_serial;
> +
> + /*
> + * callbacks that are triggered from client events.
> + * They should be called from the thread that handles the RedClient
> + */
> + channel_client_connect_proc connect;
> + channel_client_disconnect_proc disconnect;
> + channel_client_migrate_proc migrate;
> };
>
> #define FOREACH_CLIENT(_channel, _data) \
> @@ -122,7 +120,6 @@ 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);
> // 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 0dd26b22c..1a2f9f3f9 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -40,7 +40,6 @@
>
> #include "red-qxl.h"
>
> -
> #define MAX_MONITORS_COUNT 16
>
> struct QXLState {
> @@ -75,102 +74,6 @@ int red_qxl_check_qxl_version(QXLInstance *qxl, int
> major, int minor)
> ((qxl_major == major) && (qxl_minor >= minor)));
> }
>
> -static void red_qxl_set_display_peer(RedChannel *channel, RedClient *client,
> - RedStream *stream, int migration,
> - RedChannelCapabilities *caps)
> -{
> - RedWorkerMessageDisplayConnect payload = {0,};
> - Dispatcher *dispatcher;
> -
> - spice_debug("%s", "");
> - dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> "dispatcher");
> - // 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);
> - payload.stream = stream;
> - payload.migration = migration;
> - red_channel_capabilities_init(&payload.caps, caps);
> -
> - dispatcher_send_message(dispatcher,
> - RED_WORKER_MESSAGE_DISPLAY_CONNECT,
> - &payload);
> -}
> -
> -static void red_qxl_disconnect_display_peer(RedChannelClient *rcc)
> -{
> - RedWorkerMessageDisplayDisconnect payload;
> - Dispatcher *dispatcher;
> - RedChannel *channel = red_channel_client_get_channel(rcc);
> -
> - dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> "dispatcher");
> -
> - payload.rcc = rcc;
> -
> - // TODO: we turned it to be sync, due to client_destroy . Should we
> support async? - for this we will need ref count
> - // for channels
> - dispatcher_send_message(dispatcher,
> - RED_WORKER_MESSAGE_DISPLAY_DISCONNECT,
> - &payload);
> -}
> -
> -static void red_qxl_display_migrate(RedChannelClient *rcc)
> -{
> - RedWorkerMessageDisplayMigrate payload;
> - Dispatcher *dispatcher;
> - RedChannel *channel = red_channel_client_get_channel(rcc);
> -
> - dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> "dispatcher");
> - payload.rcc = g_object_ref(rcc);
> - dispatcher_send_message(dispatcher,
> - RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> - &payload);
> -}
> -
> -static void red_qxl_set_cursor_peer(RedChannel *channel, RedClient *client,
> RedStream *stream,
> - int migration,
> - RedChannelCapabilities *caps)
> -{
> - RedWorkerMessageCursorConnect payload = {0,};
> - Dispatcher *dispatcher = (Dispatcher
> *)g_object_get_data(G_OBJECT(channel), "dispatcher");
> - // 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);
> - payload.stream = stream;
> - payload.migration = migration;
> - red_channel_capabilities_init(&payload.caps, caps);
> -
> - dispatcher_send_message(dispatcher,
> - RED_WORKER_MESSAGE_CURSOR_CONNECT,
> - &payload);
> -}
> -
> -static void red_qxl_disconnect_cursor_peer(RedChannelClient *rcc)
> -{
> - RedWorkerMessageCursorDisconnect payload;
> - Dispatcher *dispatcher;
> - RedChannel *channel = red_channel_client_get_channel(rcc);
> -
> - dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> "dispatcher");
> - payload.rcc = rcc;
> -
> - dispatcher_send_message(dispatcher,
> - RED_WORKER_MESSAGE_CURSOR_DISCONNECT,
> - &payload);
> -}
> -
> -static void red_qxl_cursor_migrate(RedChannelClient *rcc)
> -{
> - RedWorkerMessageCursorMigrate payload;
> - Dispatcher *dispatcher;
> - RedChannel *channel = red_channel_client_get_channel(rcc);
> -
> - dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> "dispatcher");
> - payload.rcc = g_object_ref(rcc);
> - dispatcher_send_message(dispatcher,
> - RED_WORKER_MESSAGE_CURSOR_MIGRATE,
> - &payload);
> -}
> -
> static void red_qxl_update_area(QXLState *qxl_state, uint32_t surface_id,
> QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> uint32_t num_dirty_rects, uint32_t
> clear_dirty_region)
> @@ -914,8 +817,6 @@ size_t red_qxl_get_monitors_count(const QXLInstance *qxl)
> void red_qxl_init(RedsState *reds, QXLInstance *qxl)
> {
> QXLState *qxl_state;
> - ClientCbs client_cursor_cbs = { NULL, };
> - ClientCbs client_display_cbs = { NULL, };
>
> spice_return_if_fail(qxl != NULL);
>
> @@ -948,17 +849,7 @@ void red_qxl_init(RedsState *reds, QXLInstance *qxl)
> qxl_state->max_monitors = UINT_MAX;
> qxl->st = qxl_state;
>
> - // TODO: move to their respective channel files
> - 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_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;
> -
> - qxl_state->worker = red_worker_new(qxl, &client_cursor_cbs,
> - &client_display_cbs);
> + qxl_state->worker = red_worker_new(qxl);
>
> red_worker_run(qxl_state->worker);
> }
> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> index 78a8c9083..d8ee9128d 100644
> --- a/server/red-stream-device.c
> +++ b/server/red-stream-device.c
> @@ -695,12 +695,7 @@ stream_device_create_channel(StreamDevice *dev)
> g_return_if_fail(id >= 0);
>
> StreamChannel *stream_channel = stream_channel_new(reds, id);
> -
> CursorChannel *cursor_channel = cursor_channel_new(reds, id, core);
> - ClientCbs client_cbs = { NULL, };
> - client_cbs.connect = (channel_client_connect_proc)
> cursor_channel_connect;
> - client_cbs.migrate = cursor_channel_client_migrate;
> - red_channel_register_client_cbs(RED_CHANNEL(cursor_channel),
> &client_cbs);
>
> dev->stream_channel = stream_channel;
> dev->cursor_channel = cursor_channel;
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 3cb12b9c2..45c163193 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -53,6 +53,266 @@
> #define pthread_setname_np pthread_set_name_np
> #endif
>
> +
> +// A DisplayChannel subclass that provides virtual functions that dispatch
> the client callbacks to
> +// the worker thread.
> +typedef struct WorkerDisplayChannel
> +{
> + DisplayChannel parent;
Why in WorkerCursorChannel there's the dispatcher and here not?
Looks like this patch is incomplete.
Also there are a lot of calls from the worker to red_qxl_get_dispatcher,
is there a rationale why some functions are here and why others in
red-qxl?
Looks like part of threading is handled by RedChannel, part by RedWorker,
part by RedQxl. Is there a rationale or we have no clear idea why?
> +} WorkerDisplayChannel;
> +
> +typedef struct WorkerDisplayChannelClass
> +{
> + DisplayChannelClass parent_class;
> +} WorkerDisplayChannelClass;
> +G_DEFINE_TYPE(WorkerDisplayChannel, worker_display_channel,
> TYPE_DISPLAY_CHANNEL)
> +
> +static void red_qxl_set_display_peer(RedChannel *channel, RedClient *client,
> + RedStream *stream, int migration,
> + RedChannelCapabilities *caps)
> +{
> + RedWorkerMessageDisplayConnect payload = {0,};
> + QXLInstance *qxl;
> + g_object_get(channel, "qxl", &qxl, NULL);
> +
> + spice_debug("%s", "");
> + Dispatcher *dispatcher = red_qxl_get_dispatcher(qxl);
> + // 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);
> + payload.stream = stream;
> + payload.migration = migration;
> + red_channel_capabilities_init(&payload.caps, caps);
> +
> + dispatcher_send_message(dispatcher,
> + RED_WORKER_MESSAGE_DISPLAY_CONNECT,
> + &payload);
> +}
> +
> +static void red_qxl_disconnect_display_peer(RedChannelClient *rcc)
> +{
> + RedWorkerMessageDisplayDisconnect payload;
> + Dispatcher *dispatcher;
> + RedChannel *channel = red_channel_client_get_channel(rcc);
> + QXLInstance *qxl;
> + g_object_get(channel, "qxl", &qxl, NULL);
> +
> + dispatcher = red_qxl_get_dispatcher(qxl);
> +
> + payload.rcc = rcc;
> +
> + // TODO: we turned it to be sync, due to client_destroy . Should we
> support async? - for this we will need ref count
> + // for channels
> + dispatcher_send_message(dispatcher,
> + RED_WORKER_MESSAGE_DISPLAY_DISCONNECT,
> + &payload);
> +}
> +
> +static void red_qxl_display_migrate(RedChannelClient *rcc)
> +{
> + RedWorkerMessageDisplayMigrate payload;
> + Dispatcher *dispatcher;
> + RedChannel *channel = red_channel_client_get_channel(rcc);
> + QXLInstance *qxl;
> + g_object_get(channel, "qxl", &qxl, NULL);
> +
> + dispatcher = red_qxl_get_dispatcher(qxl);
pretty ugly I would say, not that this patch is improving this.
> + payload.rcc = g_object_ref(rcc);
> + dispatcher_send_message(dispatcher,
> + RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> + &payload);
> +}
> +
> +static void worker_display_channel_class_init(WorkerDisplayChannelClass
> *klass)
> +{
> + RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> +
> + // client callbacks
> + channel_class->connect = red_qxl_set_display_peer;
> + channel_class->disconnect = red_qxl_disconnect_display_peer;
> + channel_class->migrate = red_qxl_display_migrate;
> +}
> +
> +static void worker_display_channel_init(WorkerDisplayChannel *self)
> +{
> +}
> +
> +DisplayChannel* worker_display_channel_new(RedsState *reds,
> + QXLInstance *qxl,
> + const SpiceCoreInterfaceInternal
> *core,
> + int migrate, int stream_video,
> + GArray *video_codecs,
> + uint32_t n_surfaces)
> +{
> + DisplayChannel *display;
> +
> + display = g_object_new(worker_display_channel_get_type(),
> + "spice-server", reds,
> + "core-interface", core,
> + "channel-type", SPICE_CHANNEL_DISPLAY,
> + "id", qxl->id,
> + "migration-flags",
> + (SPICE_MIGRATE_NEED_FLUSH |
> SPICE_MIGRATE_NEED_DATA_TRANSFER),
> + "qxl", qxl,
> + "n-surfaces", n_surfaces,
> + "video-codecs", video_codecs,
> + "handle-acks", TRUE,
> + NULL);
> + if (display) {
> + display_channel_set_stream_video(display, stream_video);
> + }
> + return display;
> +}
> +
> +// a CursorChannel subclass that provides virtual functions that dispatch
> the client callbacks to
> +// the worker thread.
> +typedef struct WorkerCursorChannel {
> + CursorChannel parent;
> + Dispatcher *dispatcher;
> +} WorkerCursorChannel;
> +
> +typedef struct WorkerCursorChannelClass {
> + CursorChannelClass parent_class;
> +} WorkerCursorChannelClass;
> +
> +#define TYPE_WORKER_CURSOR_CHANNEL worker_cursor_channel_get_type()
> +#define WORKER_CURSOR_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> TYPE_WORKER_CURSOR_CHANNEL, WorkerCursorChannel))
> +#define WORKER_CURSOR_CHANNEL_CLASS(klass) \
> + (G_TYPE_CHECK_CLASS_CAST((klass), TYPE_WORKER_CURSOR_CHANNEL,
> WorkerCursorChannelClass))
> +#define IS_WORKER_CURSOR_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj),
> TYPE_WORKER_CURSOR_CHANNEL))
> +#define IS_WORKER_CURSOR_CHANNEL_CLASS(klass)
> (G_TYPE_CHECK_CLASS_TYPE((klass), TYPE_WORKER_CURSOR_CHANNEL))
> +#define WORKER_CURSOR_CHANNEL_GET_CLASS(obj) \
> + (G_TYPE_INSTANCE_GET_CLASS((obj), TYPE_WORKER_CURSOR_CHANNEL,
> WorkerCursorChannelClass))
> +
> +G_DEFINE_TYPE(WorkerCursorChannel, worker_cursor_channel,
> TYPE_CURSOR_CHANNEL)
> +
What all this boiler plate? SPICE_DECLARE_TYPE ?
> +static void worker_cursor_channel_init(WorkerCursorChannel *self) {
> +}
> +
> +enum {
> + PROP0,
> + PROP_DISPATCHER
> +};
> +
> +static void worker_cursor_channel_get_property(GObject *object,
> + guint property_id,
> + GValue *value,
> + GParamSpec *pspec)
> +{
> + WorkerCursorChannel *self = WORKER_CURSOR_CHANNEL(object);
> +
> + switch (property_id)
> + {
> + case PROP_DISPATCHER:
> + g_value_set_object(value, self->dispatcher);
> + break;
> + default:
> + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> + }
> +}
> +
> +static void worker_cursor_channel_set_property(GObject *object,
> + guint property_id,
> + const GValue *value,
> + GParamSpec *pspec)
> +{
> + WorkerCursorChannel *self = WORKER_CURSOR_CHANNEL(object);
> +
> + switch (property_id)
> + {
> + case PROP_DISPATCHER:
> + self->dispatcher = g_value_get_object(value);
> + break;
> + default:
> + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> + }
> +}
> +
> +static void red_qxl_set_cursor_peer(RedChannel *channel, RedClient *client,
> RedStream *stream,
> + int migration,
> + RedChannelCapabilities *caps)
> +{
> + RedWorkerMessageCursorConnect payload = {0,};
> + WorkerCursorChannel *cursor_channel = WORKER_CURSOR_CHANNEL(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);
> + payload.stream = stream;
> + payload.migration = migration;
> + red_channel_capabilities_init(&payload.caps, caps);
> +
> + dispatcher_send_message(cursor_channel->dispatcher,
> + RED_WORKER_MESSAGE_CURSOR_CONNECT,
> + &payload);
> +}
> +
> +static void red_qxl_disconnect_cursor_peer(RedChannelClient *rcc)
> +{
> + RedWorkerMessageCursorDisconnect payload;
> + RedChannel *channel = red_channel_client_get_channel(rcc);
> + WorkerCursorChannel *cursor_channel = WORKER_CURSOR_CHANNEL(channel);
> +
> + payload.rcc = rcc;
> +
> + dispatcher_send_message(cursor_channel->dispatcher,
> + RED_WORKER_MESSAGE_CURSOR_DISCONNECT,
> + &payload);
> +}
> +
> +static void red_qxl_cursor_migrate(RedChannelClient *rcc)
> +{
> + RedWorkerMessageCursorMigrate payload;
> + RedChannel *channel = red_channel_client_get_channel(rcc);
> + WorkerCursorChannel *cursor_channel = WORKER_CURSOR_CHANNEL(channel);
> +
> + payload.rcc = g_object_ref(rcc);
> + dispatcher_send_message(cursor_channel->dispatcher,
> + RED_WORKER_MESSAGE_CURSOR_MIGRATE,
> + &payload);
> +}
> +
> +static void worker_cursor_channel_class_init(WorkerCursorChannelClass
> *klass)
> +{
> + GObjectClass *object_class = G_OBJECT_CLASS(klass);
> + RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> +
> + object_class->get_property = worker_cursor_channel_get_property;
> + object_class->set_property = worker_cursor_channel_set_property;
> + // client callbacks
> + channel_class->connect = red_qxl_set_cursor_peer;
> + channel_class->disconnect = red_qxl_disconnect_cursor_peer;
> + channel_class->migrate = red_qxl_cursor_migrate;
> +
> + GParamSpec *spec = g_param_spec_object("dispatcher",
> + "dispatcher",
> + "The dispatcher associated with
> this channel",
> + TYPE_DISPATCHER,
> + G_PARAM_READWRITE |
> + G_PARAM_CONSTRUCT_ONLY |
> + G_PARAM_STATIC_STRINGS);
> + g_object_class_install_property(object_class, PROP_DISPATCHER, spec);
> +}
> +
> +WorkerCursorChannel* worker_cursor_channel_new(RedsState *server,
> + int id,
> + const SpiceCoreInterfaceInternal
> *core,
> + Dispatcher *dispatcher)
> +{
> + spice_debug("create QXL cursor channel");
> + WorkerCursorChannel *channel =
> g_object_new(worker_cursor_channel_get_type(),
> + "spice-server", server,
> + "core-interface", core,
> + "channel-type",
> SPICE_CHANNEL_CURSOR,
> + "id", id,
> + "dispatcher", dispatcher,
> + "migration-flags", 0,
> + "handle-acks", TRUE,
> + NULL);
> + spice_debug("created QXL cursor channel %p", channel);
> + return channel;
> +}
> +
> +
> #define CMD_RING_POLL_TIMEOUT 10 //milli
> #define CMD_RING_POLL_RETRIES 1
>
> @@ -1259,9 +1519,7 @@ static GSourceFuncs worker_source_funcs = {
> .dispatch = worker_source_dispatch,
> };
>
> -RedWorker* red_worker_new(QXLInstance *qxl,
> - const ClientCbs *client_cursor_cbs,
> - const ClientCbs *client_display_cbs)
> +RedWorker* red_worker_new(QXLInstance *qxl)
> {
> QXLDevInitInfo init_info;
> RedWorker *worker;
> @@ -1316,22 +1574,18 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>
> worker->event_timeout = INF_EVENT_WAIT;
>
> - worker->cursor_channel = cursor_channel_new(reds, qxl->id,
> - &worker->core);
> + worker->cursor_channel = CURSOR_CHANNEL(worker_cursor_channel_new(reds,
> qxl->id,
> +
> &worker->core,
> dispatcher));
> 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);
>
> // TODO: handle seamless migration. Temp, setting migrate to FALSE
> - worker->display_channel = display_channel_new(reds, qxl, &worker->core,
> FALSE,
> -
> reds_get_streaming_video(reds),
> -
> reds_get_video_codecs(reds),
> - init_info.n_surfaces);
> + worker->display_channel = worker_display_channel_new(reds, qxl,
> &worker->core, FALSE,
> +
> reds_get_streaming_video(reds),
> +
> reds_get_video_codecs(reds),
> +
> init_info.n_surfaces);
> 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);
>
> return worker;
> }
> diff --git a/server/red-worker.h b/server/red-worker.h
> index 767329da5..958f9203d 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -28,9 +28,7 @@
>
> typedef struct RedWorker RedWorker;
>
> -RedWorker* red_worker_new(QXLInstance *qxl,
> - const ClientCbs *client_cursor_cbs,
> - const ClientCbs *client_display_cbs);
> +RedWorker* red_worker_new(QXLInstance *qxl);
> bool red_worker_run(RedWorker *worker);
> void red_worker_free(RedWorker *worker);
>
> diff --git a/server/smartcard.c b/server/smartcard.c
> index 9ccfbc6b1..e3e746cab 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -535,13 +535,9 @@ red_smartcard_channel_constructed(GObject *object)
> {
> RedSmartcardChannel *self = RED_SMARTCARD_CHANNEL(object);
> RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> - ClientCbs client_cbs = { NULL, };
>
> G_OBJECT_CLASS(red_smartcard_channel_parent_class)->constructed(object);
>
> - client_cbs.connect = smartcard_connect_client;
> - red_channel_register_client_cbs(RED_CHANNEL(self), &client_cbs);
> -
> reds_register_channel(reds, RED_CHANNEL(self));
> }
>
> @@ -559,6 +555,8 @@ red_smartcard_channel_class_init(RedSmartcardChannelClass
> *klass)
> channel_class->handle_migrate_flush_mark =
> smartcard_channel_client_handle_migrate_flush_mark;
> channel_class->handle_migrate_data =
> smartcard_channel_client_handle_migrate_data;
>
> + // client callbacks
> + channel_class->connect = smartcard_connect_client;
> }
>
> static void smartcard_init(RedsState *reds)
> diff --git a/server/sound.c b/server/sound.c
> index 44b27dec3..167f68c57 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -1377,16 +1377,11 @@ playback_channel_init(PlaybackChannel *self)
> static void
> playback_channel_constructed(GObject *object)
> {
> - ClientCbs client_cbs = { NULL, };
> SndChannel *self = SND_CHANNEL(object);
> RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
>
> G_OBJECT_CLASS(playback_channel_parent_class)->constructed(object);
>
> - client_cbs.connect = snd_set_playback_peer;
> - client_cbs.migrate = snd_migrate_channel_client;
> - red_channel_register_client_cbs(RED_CHANNEL(self), &client_cbs);
> -
> if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1,
> SND_CODEC_ANY_FREQUENCY)) {
> red_channel_set_cap(RED_CHANNEL(self),
> SPICE_PLAYBACK_CAP_CELT_0_5_1);
> }
> @@ -1407,6 +1402,10 @@ playback_channel_class_init(PlaybackChannelClass
> *klass)
> channel_class->parser =
> spice_get_client_channel_parser(SPICE_CHANNEL_PLAYBACK, NULL);
> channel_class->handle_message = red_channel_client_handle_message;
> channel_class->send_item = playback_channel_send_item;
> +
> + // client callbacks
> + channel_class->connect = snd_set_playback_peer;
> + channel_class->migrate = snd_migrate_channel_client;
> }
>
> void snd_attach_playback(RedsState *reds, SpicePlaybackInstance *sin)
> @@ -1427,16 +1426,11 @@ record_channel_init(RecordChannel *self)
> static void
> record_channel_constructed(GObject *object)
> {
> - ClientCbs client_cbs = { NULL, };
> SndChannel *self = SND_CHANNEL(object);
> RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
>
> G_OBJECT_CLASS(record_channel_parent_class)->constructed(object);
>
> - client_cbs.connect = snd_set_record_peer;
> - client_cbs.migrate = snd_migrate_channel_client;
> - red_channel_register_client_cbs(RED_CHANNEL(self), &client_cbs);
> -
> if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1,
> SND_CODEC_ANY_FREQUENCY)) {
> red_channel_set_cap(RED_CHANNEL(self), SPICE_RECORD_CAP_CELT_0_5_1);
> }
> @@ -1457,6 +1451,10 @@ record_channel_class_init(RecordChannelClass *klass)
> channel_class->parser =
> spice_get_client_channel_parser(SPICE_CHANNEL_RECORD, NULL);
> channel_class->handle_message = record_channel_handle_message;
> channel_class->send_item = record_channel_send_item;
> +
> + // client callbacks
> + channel_class->connect = snd_set_record_peer;
> + channel_class->migrate = snd_migrate_channel_client;
> }
>
> void snd_attach_record(RedsState *reds, SpiceRecordInstance *sin)
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 51550c1a0..d094166ee 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -185,14 +185,10 @@ static void
> red_vmc_channel_constructed(GObject *object)
> {
> RedVmcChannel *self = RED_VMC_CHANNEL(object);
> - ClientCbs client_cbs = { NULL, };
> RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
>
> G_OBJECT_CLASS(red_vmc_channel_parent_class)->constructed(object);
>
> - client_cbs.connect = spicevmc_connect;
> - red_channel_register_client_cbs(RED_CHANNEL(self), &client_cbs);
> -
> red_channel_init_stat_node(RED_CHANNEL(self), NULL, "spicevmc");
> const RedStatNode *stat = red_channel_get_stat_node(RED_CHANNEL(self));
> stat_init_counter(&self->in_data, reds, stat, "in_data", TRUE);
> @@ -723,6 +719,9 @@ red_vmc_channel_class_init(RedVmcChannelClass *klass)
> channel_class->send_item = spicevmc_red_channel_send_item;
> channel_class->handle_migrate_flush_mark =
> spicevmc_channel_client_handle_migrate_flush_mark;
> channel_class->handle_migrate_data =
> spicevmc_channel_client_handle_migrate_data;
> +
> + // client callbacks
> + channel_class->connect = spicevmc_connect;
> }
>
> static void
> diff --git a/server/stream-channel.c b/server/stream-channel.c
> index c38ee0430..ed844d6da 100644
> --- a/server/stream-channel.c
> +++ b/server/stream-channel.c
> @@ -443,15 +443,11 @@ stream_channel_connect(RedChannel *red_channel,
> RedClient *red_client, RedStream
> static void
> stream_channel_constructed(GObject *object)
> {
> - ClientCbs client_cbs = { NULL, };
> RedChannel *red_channel = RED_CHANNEL(object);
> RedsState *reds = red_channel_get_server(red_channel);
>
> G_OBJECT_CLASS(stream_channel_parent_class)->constructed(object);
>
> - client_cbs.connect = stream_channel_connect;
> - red_channel_register_client_cbs(red_channel, &client_cbs);
> -
> red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
>
> @@ -470,6 +466,7 @@ stream_channel_class_init(StreamChannelClass *klass)
> channel_class->handle_message = handle_message;
>
> channel_class->send_item = stream_channel_send_item;
> + channel_class->connect = stream_channel_connect;
> }
>
> static void
> diff --git a/server/tests/test-channel.c b/server/tests/test-channel.c
> index 1c9148df8..9700e31c1 100644
> --- a/server/tests/test-channel.c
> +++ b/server/tests/test-channel.c
> @@ -105,25 +105,14 @@ test_connect_client(RedChannel *channel, RedClient
> *client, RedStream *stream,
> }
> }
>
> -static void
> -red_test_channel_constructed(GObject *object)
> -{
> - G_OBJECT_CLASS(red_test_channel_parent_class)->constructed(object);
> -
> - ClientCbs client_cbs = { .connect = test_connect_client, };
> - red_channel_register_client_cbs(RED_CHANNEL(object), &client_cbs);
> -}
> -
> static void
> red_test_channel_class_init(RedTestChannelClass *klass)
> {
> - GObjectClass *object_class = G_OBJECT_CLASS(klass);
> - object_class->constructed = red_test_channel_constructed;
> -
> RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> channel_class->parser =
> spice_get_client_channel_parser(SPICE_CHANNEL_PORT, NULL);
> channel_class->handle_message = red_channel_client_handle_message;
> channel_class->send_item = test_channel_send_item;
> + channel_class->connect = test_connect_client;
> }
>
> static uint8_t *
The rationale is "these callback should be vfuncs" but this patch
creates additional classes to allow to change these function for specific
purposes which is the opposite of the rational.
Yesterday (evening/night for me) we had some discussion about this
"small" change (in theory) brought so many "not really clean" changes.
I came up with a follow up series integration most of the discussed
changes.
Frediano
More information about the Spice-devel
mailing list