[Spice-devel] [RFC PATCH spice-server] stream-device: Create channels before first not main connection
Christophe de Dinechin
cdupontd at redhat.com
Fri Feb 23 11:12:20 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?
>
>>>
>>> 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)
>>> {
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list