[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.

It turns out, there is. MainChannel is special and is not registered
with 'reds' like the other channels are. In fact, when channels are
registered, we advertise the new channel on the main channel, so it is
assumed that the main channel already exists before the first call to
reds_register_channel().


> Would make it sense to move the registration to RedChannel?

Since we can't register the main channel without changing some
assumptions (the main channel obviously cannot announce itself as a new
channel), I'm not sure it makes sense to move this registration down to
RedChannel.


> However I would implement it with a Initiable interface, registering
> a channel while constructing it does not seem a good idea.

After looking at it, I'm not sure that I agree with implementing the
Initable interface either. GInitable allows you to implement an object
for which simple allocation is not sufficient to use the object. In
other words, the object may be invalid after allocation if there is a
failure during an initialization step. But reds_register_channel()
cannot fail -- it has a 'void' return value. So it seems that making it
Initable just complicates the type heirarchy for no real benefit.


> 
> 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.
> 
> Frediano