[Spice-devel] [RFC PATCH spice-server v4 02/22] Notify client of the creation of new channels dynamically
Jonathon Jongsma
jjongsma at redhat.com
Fri Aug 25 16:36:40 UTC 2017
On Fri, 2017-08-25 at 10:53 +0100, Frediano Ziglio wrote:
> This allows to add channels after the client is connected.
"allows to add" -> "allows the server to add"
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/main-channel-client.c | 50
> ++++++++++++++++++++++++++++++++++++++++++++
> server/main-channel-client.h | 3 +++
> server/main-channel.c | 6 ++++++
> server/main-channel.h | 3 +++
> server/reds.c | 2 ++
> 5 files changed, 64 insertions(+)
>
> diff --git a/server/main-channel-client.c b/server/main-channel-
> client.c
> index 82b578c8..25c7c13e 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -119,6 +119,11 @@ typedef struct RedMultiMediaTimePipeItem {
> int time;
> } RedMultiMediaTimePipeItem;
>
> +typedef struct RedRegisterChannelPipeItem {
> + RedPipeItem base;
> + RedChannel *channel;
> +} RedRegisterChannelPipeItem;
> +
> #define ZERO_BUF_SIZE 4096
>
> static const uint8_t zero_page[ZERO_BUF_SIZE] = {0};
> @@ -437,6 +442,26 @@ RedPipeItem
> *main_multi_media_time_item_new(RedChannelClient *rcc,
> return &item->base;
> }
>
> +static void register_channel_pipe_item_free(RedPipeItem *base)
> +{
> + RedRegisterChannelPipeItem *pipe_item =
> SPICE_UPCAST(RedRegisterChannelPipeItem, base);
> +
> + g_object_unref(pipe_item->channel);
> + free(pipe_item);
> +}
> +
> +RedPipeItem *register_channel_item_new(RedChannelClient *rcc, void
> *data, int num)
> +{
> + RedChannel *channel = data;
> + RedRegisterChannelPipeItem *item;
> +
> + item = spice_new0(RedRegisterChannelPipeItem, 1);
> + red_pipe_item_init_full(&item->base,
> RED_PIPE_ITEM_TYPE_MAIN_REGISTERED_CHANNEL,
> + register_channel_pipe_item_free);
> + item->channel = g_object_ref(channel);
> + return &item->base;
> +}
> +
> void main_channel_client_handle_migrate_connected(MainChannelClient
> *mcc,
> int success,
> int seamless)
> @@ -918,6 +943,27 @@ static void
> main_channel_marshall_agent_connected(SpiceMarshaller *m,
> spice_marshall_msg_main_agent_connected_tokens(m, &connected);
> }
>
> +static void
> main_channel_marshall_registered_channel(RedChannelClient *rcc,
> + SpiceMarshaller
> *m,
> + RedRegisterChan
> nelPipeItem *item)
> +{
> + SpiceMsgChannels* channels_info;
> + RedChannel *channel = item->channel;
> +
> + red_channel_client_init_send_data(rcc,
> SPICE_MSG_MAIN_CHANNELS_LIST);
> + channels_info = (SpiceMsgChannels
> *)spice_malloc(sizeof(SpiceMsgChannels)
> + + 1 * sizeof(SpiceChannelId));
> +
> + uint32_t type, id;
> + g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> + channels_info->channels[0].type = type;
> + channels_info->channels[0].id = id;
> + channels_info->num_of_channels = 1;
> +
> + spice_marshall_msg_main_channels_list(m, channels_info);
> + free(channels_info);
> +}
It's a little bit surprising to me that this works. I had always
assumed (based on the name) that this protocol message was supposed to
send a list *all* of the channels. I looked at the spice-gtk code and
it appears that it simply creates a new client-side channel for each
channel in the list, without even verifying whether a channel with that
ID already exists or not. So when we get a new CHANNELS_LIST message
with a single channel, it creates a new client channel of that type.
Presumably this is because in the past this message was only sent once
at startup so since we hadn't created any channels yet, we could assume
that all channels in the list were by definition new.
The protocol documentation (docs/Spice_protocol.odt) is a bit vague on
this message. It says that it can only be sent after receiving a
REDC_MAIN_ATTACH_CHANNELS message, but doesn't say anything about
whether the message can be sent multiple times in response to a single
ATTACH_CHANNELS message from the client.
Given the requirement above, this patch potentially introduces a
protocol violation. If the new channel happens to be created after a
client connects, but before the client sends an ATTACH_CHANNELS
message, we would violate that rule. We'd also probably introduce some
client misbehavior in this corner case, such as:
-> CHANNELS_LIST (in response to new display channel)
[ client creates a channel object for the single new display
channel ]
<- ATTACH_CHANNELS
-> CHANNELS_LIST (all channels including new display channel)
[ client creates channel object for all channels, including a
duplicate channel object for the new display channel ]
> +
> void main_channel_client_send_item(RedChannelClient *rcc,
> RedPipeItem *base)
> {
> MainChannelClient *mcc = MAIN_CHANNEL_CLIENT(rcc);
> @@ -994,6 +1040,10 @@ void
> main_channel_client_send_item(RedChannelClient *rcc, RedPipeItem
> *base)
> case RED_PIPE_ITEM_TYPE_MAIN_AGENT_CONNECTED_TOKENS:
> main_channel_marshall_agent_connected(m, rcc, base);
> break;
> + case RED_PIPE_ITEM_TYPE_MAIN_REGISTERED_CHANNEL:
> + main_channel_marshall_registered_channel(rcc, m,
> + SPICE_UPCAST(RedRegisterChannelPipeItem, base));
> + break;
> default:
> break;
> };
> diff --git a/server/main-channel-client.h b/server/main-channel-
> client.h
> index a2e38c2f..6911cb07 100644
> --- a/server/main-channel-client.h
> +++ b/server/main-channel-client.h
> @@ -122,6 +122,7 @@ enum {
> RED_PIPE_ITEM_TYPE_MAIN_NAME,
> RED_PIPE_ITEM_TYPE_MAIN_UUID,
> RED_PIPE_ITEM_TYPE_MAIN_AGENT_CONNECTED_TOKENS,
> + RED_PIPE_ITEM_TYPE_MAIN_REGISTERED_CHANNEL,
> };
>
> typedef struct MainMouseModeItemInfo {
> @@ -138,6 +139,8 @@ typedef struct MainMultiMediaTimeItemInfo {
> RedPipeItem *main_multi_media_time_item_new(RedChannelClient *rcc,
> void *data, int num);
>
> +RedPipeItem *register_channel_item_new(RedChannelClient *rcc, void
> *data, int num);
> +
> G_END_DECLS
>
> #endif /* MAIN_CHANNEL_CLIENT_H_ */
> diff --git a/server/main-channel.c b/server/main-channel.c
> index 4834f79b..f9be9e6c 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -170,6 +170,12 @@ static void
> main_channel_fill_mig_target(MainChannel *main_channel, RedsMigSpice
> main_channel->mig_target.sport = mig_target->sport;
> }
>
> +void
> +main_channel_registered_new_channel(MainChannel *main_chan,
> RedChannel *channel)
> +{
> + red_channel_pipes_new_add(RED_CHANNEL(main_chan),
> register_channel_item_new, channel);
> +}
I'll just note here that this is called for *all* new channels, so
looks like the server will send out a new single-channel CHANNELS_LIST
message for every channel that is created. But generally channels are
created before a client is ever connected, and
red_channel_pipes_new_add() will simply drop the pipe item if there are
no clients connected.
> +
> void main_channel_migrate_switch(MainChannel *main_chan,
> RedsMigSpice *mig_target)
> {
> main_channel_fill_mig_target(main_chan, mig_target);
> diff --git a/server/main-channel.h b/server/main-channel.h
> index 833957dd..d2bdcc35 100644
> --- a/server/main-channel.h
> +++ b/server/main-channel.h
> @@ -66,6 +66,9 @@ void main_channel_push_mouse_mode(MainChannel
> *main_chan, SpiceMouseMode current
> void main_channel_push_agent_connected(MainChannel *main_chan);
> void main_channel_push_agent_disconnected(MainChannel *main_chan);
> void main_channel_push_multi_media_time(MainChannel *main_chan, int
> time);
> +/* tell MainChannel we have a new channel ready */
> +void main_channel_registered_new_channel(MainChannel *main_chan,
> + RedChannel *channel);
>
> int main_channel_is_connected(MainChannel *main_chan);
>
> diff --git a/server/reds.c b/server/reds.c
> index 005ead47..b712c182 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -387,6 +387,8 @@ void reds_register_channel(RedsState *reds,
> RedChannel *channel)
> {
> spice_assert(reds);
> reds->channels = g_list_prepend(reds->channels, channel);
> + // create new channel in the client if possible
> + main_channel_registered_new_channel(reds->main_channel,
> channel);
> }
This introduces a implicit restriction on creation order of channels.
The main channel must be created first or we will crash. I personally
think it would be cleaner for the SpiceServer object to emit a "new-
channel" signal when a new channel is registered. Then MainChannel
could connect to this signal and send out updates when it is triggered.
But at the moment SpiceServer is not a GObject so it's kind of a moot
point.
>
> void reds_unregister_channel(RedsState *reds, RedChannel *channel)
More information about the Spice-devel
mailing list