[Spice-devel] [RFC PATCH spice-server] stream-device: Create channels before first not main connection
Frediano Ziglio
fziglio at redhat.com
Tue Feb 27 10:57:40 UTC 2018
>
> > On 23 Feb 2018, at 10:24, Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> >>
> >> 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”
>
> Not better, the problem is “not main”. Maybe “non-main”?
>
> >
> >> 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.
>
> What confuses me is that you mention it’s a “temporary” solution. Why is it
> temporary? Because it is not complete (i.e. partial)? Because it is a quick
> fix until we figure out a better solution? Because something will break it
> later?
>
Is written: "however this requires some protocol changes and different
security considerations".
Frediano
> >
> >>>
> >>> 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 */
>
> Based on the rest of the discussion, explain the rationale in the comment:
>
> * This should be called immediately after main channel creation when a client
> connects
> * in order to open streaming channels before a possible ticket timeout
> * (waiting for the agent to spin up in the guest may exceed ticket timeout)
>
>
>
> >>> +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.
>
> Hmmm. Why does this have to be an external API at all?
>
> Would it work if we did it later, i.e. outside of reds_late_initiailization?
> It looks to me like the whole rationale of this patch is that it would not.
>
>
> >
> >> 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*.
>
> Well, since they were in the same patch, it was somewhat obvious after I read
> the stream_device_create_channel function body. But not before. However,
> with the comment I added above, then I think it’s OK.
>
> >
> >>> + 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)
> >>> {
More information about the Spice-devel
mailing list