[Spice-devel] [RFC PATCH spice-server] stream-device: Create channels before first not main connection

[Christophe de Dinechin]

Title of the patch: “first not main” sounds un-english because it mixes adjective and noun-used-as-adjective. Contrast “first, not last, connexion” (with commas, I believe). Jonathon, any suggestion? Is “first non-primary” correct, or does “primary” mean something else for SPICE?

Overall comment on the commit log: I don’t understand the problem nor the fix from the log. What causes such a delay between the creation of channels that could cause a ticket to expire? I understood from the discussion yesterday that this was the time between the SPICE connexion (which may happen at boot time) and the launch of the streaming agent (which may have to wait until run level 5 is reached). So I believe the solution here is to let the client open the streaming channels, assuming there will be a streaming agent showing up later to provide the data, this simultaneous opening of all channels for the client ensuring that there is no delay. Is my understanding correct? (it does not derive from the log but from the discussion yesterday)

Assuming I got it somewhat right, I would add that reason for the long delay to the commit log, as well as indications of how long a ticket stays valid and in which context (can this happen in RHEL, for example?)


> On Feb 23, 2018, at 9:17 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> Due to ticket expiration is possible that the streaming

expiration, it is (comma, “it")

> channels for the client are created after the ticket expires.

could you qualify “ticket” (what type of ticket, who issues it)

> This would cause the client to not be able to connect back
> to a new streaming channel.

Maybe rephrase to explain that authentication would fail?

> To avoid this create the channels when the first main channel

Toi avoid this, create (missing comma)

> connection is made. This make sure that client will connect
> to all streaming 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.

That sentence as written confuses me more than it helps. Is it a temporary solution, or a partial solution?

> 
> 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(+)
> 
> 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 */
> +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) {
> +        stream_device_create_channel(dev);
> +    }

Is it really worth splitting the if and the creation in a separate function?

As written, it looks like we are creating streaming channels for every device, and the comment is required. If the ‘if’ was in there, the intent of the loop would probably be clearer.

> +    reds->late_initialization_done = true;
> +}
> +
> static void
> red_channel_capabilities_init_from_link_message(RedChannelCapabilities *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)
> {
> -- 
> 2.14.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel