[Spice-devel] [PATCH spice-server v2 2/2] stream-device: Create channels before first non-main channel connection

[Frediano Ziglio]

> Hey,
> 
> I assume the client is not going to show an unwanted window or something
> like that?
> 

No, the client window are shown when there's a surface.

> Looks good to me, Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> though maybe people more familiar with the streaming channel will want
> to take a look too.
> 
> Christophe
> 
> On Wed, Mar 07, 2018 at 08:26:16AM +0000, Frediano Ziglio wrote:
> > Due to ticket expiration, it is possible that the streaming channels for
> > the client are created after the ticket expires. Currently, streaming
> > channels are created dynamically when the guest starts streaming to the
> > server, which can happen at any time (for instance if you decide to start
> > the graphic server manually).
> > If the ticket has expired before the streaming channel is created,
> > authentication will fail and the client will not be able to connect.
> > To avoid this, create the channels when the first main channel connection
> > is made. This ensures that client will connect to all streaming channels.
> > This could be considered a temporary solution. There may be other
> > situations where it would be useful to connect new channels after the
> > ticket has expired, but enabling this behavior would require protocol
> > changes and a careful analysis of security implications.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/reds-private.h  |  1 +
> >  server/reds.c          | 22 ++++++++++++++++++++++
> >  server/stream-device.c |  6 +++---
> >  server/stream-device.h |  1 +
> >  4 files changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/server/reds-private.h b/server/reds-private.h
> > index adc48ba5..920edc5c 100644
> > --- a/server/reds-private.h
> > +++ b/server/reds-private.h
> > @@ -117,6 +117,7 @@ struct RedsState {
> >      RedStatFile *stat_file;
> >  #endif
> >      int allow_multiple_clients;
> > +    bool late_initialization_done;
> >  
> >      /* Intermediate state for on going monitors config message from a
> >      single
> >       * client, being passed to the guest */
> > diff --git a/server/reds.c b/server/reds.c
> > index 9660476c..73c9ec20 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -1734,6 +1734,26 @@ static RedClient *reds_get_client(RedsState *reds)
> >      return reds->clients->data;
> >  }
> >  
> > +/* Performs late initializations steps.
> > + * This should be called when a client connects */
> > +static void reds_late_initialization(RedsState *reds)
> > +{
> > +    RedCharDevice *dev;
> > +
> > +    // do only once
> > +    if (reds->late_initialization_done) {
> > +        return;
> > +    }
> > +
> > +    // create stream channels for streaming devices
> > +    GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
> > +        if (IS_STREAM_DEVICE(dev)) {
> > +            stream_device_create_channel(STREAM_DEVICE(dev));
> > +        }
> > +    }
> > +    reds->late_initialization_done = true;
> > +}
> > +
> >  static void
> >  red_channel_capabilities_init_from_link_message(RedChannelCapabilities
> >  *caps,
> >                                                  const SpiceLinkMess
> >                                                  *link_mess)
> > @@ -1769,6 +1789,8 @@ static void reds_handle_main_link(RedsState *reds,
> > RedLinkInfo *link)
> >      spice_debug("trace");
> >      spice_assert(reds->main_channel);
> >  
> > +    reds_late_initialization(reds);
> > +
> >      link_mess = link->link_mess;
> >      if (!reds->allow_multiple_clients) {
> >          reds_disconnect(reds);
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index fd73e784..6cf29d37 100644
> > --- a/server/stream-device.c
> > +++ b/server/stream-device.c
> > @@ -538,8 +538,8 @@ stream_device_finalize(GObject *object)
> >      dev->msg_pos = 0;
> >  }
> >  
> > -static void
> > -allocate_channels(StreamDevice *dev)
> > +void
> > +stream_device_create_channel(StreamDevice *dev)
> >  {
> >      if (dev->stream_channel) {
> >          return;
> > @@ -600,7 +600,7 @@ stream_device_port_event(RedCharDevice *char_dev,
> > uint8_t event)
> >      // reset device and channel on close/open
> >      dev->opened = (event == SPICE_PORT_EVENT_OPENED);
> >      if (dev->opened) {
> > -        allocate_channels(dev);
> > +        stream_device_create_channel(dev);
> >      }
> >      dev->hdr_pos = 0;
> >      dev->msg_pos = 0;
> > diff --git a/server/stream-device.h b/server/stream-device.h
> > index 501629c1..27efaeab 100644
> > --- a/server/stream-device.h
> > +++ b/server/stream-device.h
> > @@ -43,6 +43,7 @@ typedef struct StreamDeviceClass StreamDeviceClass;
> >  
> >  GType stream_device_get_type(void) G_GNUC_CONST;
> >  StreamDevice *stream_device_connect(RedsState *reds,
> >  SpiceCharDeviceInstance *sin);
> > +void stream_device_create_channel(StreamDevice *dev);
> >  
> >  G_END_DECLS
> >  

Frediano