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

[Jonathon Jongsma]

On Tue, 2018-02-27 at 10:59 +0000, Frediano Ziglio wrote:
> Due to ticket expiration, is possible that the streaming channels for

"it is possible"

> the
> client are created after the ticket expires, this as currently 

end sentence after "expires". Then:

"Currently, streaming channels are..."


> 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 

add comma: "server, which can"...

> start
> the graphic server manually).
> This causes the client to not be able to connect to a new streaming
> channel as the authentication would fail.

Perhaps rephrase the previous sentence as:

"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 make sure that client will connect to all streaming 

"make" -> "makes"

or better: "This ensures that"...

> channels.
> This can be considered a temporary solution, connecting back to the
> VM is
> helpful in different situations however this requires some protocol
> changes
> and different security considerations.

I find this sentence a little bit unclear. Suggestion:

"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/char-device.h   |  1 +
>  server/reds-private.h  |  1 +
>  server/reds.c          | 20 ++++++++++++++++++++
>  server/stream-device.c | 11 +++++++++++
>  4 files changed, 33 insertions(+)
> 
> Changes since v1:
> - improve commit message.
> 
> diff --git a/server/char-device.h b/server/char-device.h
> index 54a1ef93..a9634383 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -237,6 +237,7 @@ RedCharDevice *spicevmc_device_connect(RedsState
> *reds,
>  void spicevmc_device_disconnect(RedsState *reds,
>                                  SpiceCharDeviceInstance
> *char_device);
>  RedCharDevice *stream_device_connect(RedsState *reds,
> SpiceCharDeviceInstance *sin);
> +void stream_device_create_channel(RedCharDevice *dev);
>  
>  SpiceCharDeviceInterface
> *spice_char_device_get_interface(SpiceCharDeviceInstance *instance);
>  
> 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 a31ed4e9..30243f47 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -1733,6 +1733,24 @@ static RedClient *reds_get_client(RedsState
> *reds)
>      return reds->clients->data;
>  }
>  
> +/* Performs late initializations steps.
> + * This should be called when a client connects */

C3D suggested additional comments here, which I basically agree with,
although they could be placed within the body of the function below...


> +static void reds_late_initialization(RedsState *reds)
> +{
> +    RedCharDevice *dev;
> +
> +    // do only once
> +    if (reds->late_initialization_done) {
> +        return;
> +    }
> +
> +    // create stream channels for streaming devices

here

> +    GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
> +        stream_device_create_channel(dev);
> +    }
> +    reds->late_initialization_done = true;
> +}
> +
>  static void
>  red_channel_capabilities_init_from_link_message(RedChannelCapabiliti
> es *caps,
>                                                  const SpiceLinkMess
> *link_mess)
> @@ -1768,6 +1786,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 b206b116..9298d16e 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -32,6 +32,8 @@
>      (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_STREAM_DEVICE,
> StreamDevice))
>  #define STREAM_DEVICE_CLASS(klass) \
>      (G_TYPE_CHECK_CLASS_CAST((klass), TYPE_STREAM_DEVICE,
> StreamDeviceClass))
> +#define IS_STREAM_DEVICE(obj) \
> +    (G_TYPE_CHECK_INSTANCE_TYPE((obj), TYPE_STREAM_DEVICE))
>  #define STREAM_DEVICE_GET_CLASS(obj) \
>      (G_TYPE_INSTANCE_GET_CLASS((obj), TYPE_STREAM_DEVICE,
> StreamDeviceClass))
>  
> @@ -70,6 +72,7 @@ static StreamDevice
> *stream_device_new(SpiceCharDeviceInstance *sin, RedsState *
>  G_DEFINE_TYPE(StreamDevice, stream_device, RED_TYPE_CHAR_DEVICE)
>  
>  static void char_device_set_state(RedCharDevice *char_dev, int
> state);
> +static void allocate_channels(StreamDevice *dev);
>  
>  typedef bool StreamMsgHandler(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>      SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -520,6 +523,14 @@ stream_device_connect(RedsState *reds,
> SpiceCharDeviceInstance *sin)
>      return RED_CHAR_DEVICE(dev);
>  }
>  
> +void
> +stream_device_create_channel(RedCharDevice *char_dev)
> +{
> +    if (IS_STREAM_DEVICE(char_dev)) {
> +        allocate_channels(STREAM_DEVICE(char_dev));
> +    }
> +}
> +
>  static void
>  stream_device_dispose(GObject *object)
>  {


Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>