On Wed, 2019-02-20 at 07:08 -0500, Frediano Ziglio wrote:
> >
> > For the Display Channel and the Cursor channel, move the call to
> > reds_register_channel() to the _constructed() vfunc rather than
> > calling
> > it explicitly from RedWorker. This matches what other channels do.
>
> I think this was different for these channels as they where created
> in a different thread.
> Is there a channel not registered to "reds"? I think not.
> Would make it sense to move the registration to RedChannel?
> However I would implement it with a Initiable interface, registering
> a channel while constructing it does not seem a good idea.
Good points, I'll try to rework it.
>
> Also would be good to update docs/spice_threading_model.txt
> documenting
> that creating/shutdown should be handled in the main thread.
>
> > ---
> > server/cursor-channel.c | 12 ++++++++++++
> > server/display-channel.c | 2 ++
> > server/red-worker.c | 2 --
> > 3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index 9dd69fa25..4220084f5 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -366,12 +366,24 @@ cursor_channel_finalize(GObject *object)
> > G_OBJECT_CLASS(cursor_channel_parent_class)->finalize(object);
> > }
> >
> > +static void
> > +cursor_channel_constructed(GObject *object)
> > +{
> > + RedChannel *red_channel = RED_CHANNEL(object);
> > + RedsState *reds = red_channel_get_server(red_channel);
> > +
> > + G_OBJECT_CLASS(cursor_channel_parent_class)-
> > >constructed(object);
> > +
> > + reds_register_channel(reds, red_channel);
> > +}
> > +
> > static void
> > cursor_channel_class_init(CursorChannelClass *klass)
> > {
> > GObjectClass *object_class = G_OBJECT_CLASS(klass);
> > RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> >
> > + object_class->constructed = cursor_channel_constructed;
> > object_class->finalize = cursor_channel_finalize;
> >
> > channel_class->parser =
> > spice_get_client_channel_parser(SPICE_CHANNEL_CURSOR, NULL);
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index e68ed10f8..6684135eb 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2304,6 +2304,8 @@ display_channel_constructed(GObject *object)
> > red_channel_set_cap(channel,
> > SPICE_DISPLAY_CAP_PREF_COMPRESSION);
> > red_channel_set_cap(channel,
> > SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE);
> > red_channel_set_cap(channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> > +
> > + reds_register_channel(reds, channel);
> > }
> >
> > void display_channel_process_surface_cmd(DisplayChannel *display,
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index c74ae8886..3cb12b9c2 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -1322,7 +1322,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > 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 seamless migration. Temp, setting migrate to
> > FALSE
> > worker->display_channel = display_channel_new(reds, qxl,
> > &worker->core,
> > FALSE,
> > @@ -1333,7 +1332,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > 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;
> > }
>
> Otherwise patch seems good, but I didn't test it.
> In particular I think CursorChannel is also registered in the
> StreamDevice so will be registered twice.
> There should be a test for double registration.
hmm, I'll check into it.
Thanks,
Jonathon
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel