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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel