[Spice-devel] [PATCH spice-server] Move channel registration to constructed vfunc

[Jonathon Jongsma]

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