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

[Frediano Ziglio]

> 
> 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?
> 

main is the type of channel, maybe more clear

"Create channels before first not main channel connection"

> 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?)
> 

More or less they are currently created dynamically, I'll try to add this.

> 
> > 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)
> 

ticket is a SPICE terminology, can be used by upper layers for authentication,
they are mainly made by a password and an expiration.

> > 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?
> 

Ok

> > 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?
> 

Dynamically channel creation can be implemented to support tickets.
I cannot understand why my sentence could imply partial.

> > 
> > 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?
> 

I think so, is more clear that is doing type checking and is an external API.

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

As I read the "stream_device" prefix is enough to understand that this
affects only these devices, the question that I would ask is "why this
do not possibly fail?" but I can understand easily from the fact that code
compiles and that the parameter if RedCharDevice* and not StreamDevice*.

> > +    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)
> > {

Frediano