[Spice-devel] [RFC PATCH spice-common v2 01/20] Add a monitors config message from the client to the server
Lukáš Hrázký
lhrazky at redhat.com
Thu Aug 23 09:20:21 UTC 2018
On Wed, 2018-08-22 at 11:00 -0500, Jonathon Jongsma wrote:
> On Wed, 2018-08-22 at 13:35 +0200, Lukáš Hrázký wrote:
> > On Tue, 2018-08-21 at 16:35 -0500, Jonathon Jongsma wrote:
> > > So I sent my comments on patch 04/20 before this one, but this is
> > > obviously very closely related. This spice protocol message type is
> > > being added to support that new vdagent message. This is a pretty
> > > significant change (from a protocol point of view), because in
> > > addition
> > > to adding the new id data pair to the message, the message itself
> > > has
> > > also been promoted from a vdagent message to a full-fledged main
> > > channel message. I presume this was done because the main channel
> > > has
> > > to do some processing of the data before passing it on to the
> > > agent[*].
> >
> > Yes. The messages are now different, this one has the (channel_id,
> > monitor_id) ID pair, the other one the guest_output_id (or nothing,
> > if
> > we keep the old one).
> >
> > Regardless, you need to do the processing...
>
> Yes, I was not suggesting that we can avoid the processing. I was just
> explaining what I assumed was your justification formaking this change,
> since it wasn't really explained in the commit log.
I'll add it to the commit log.
> > > But this is slightly problematic because this message still
> > > requires
> > > the vdagent to be connected in order for it to work properly. But
> > > if
> > > you simply look at the protocol message definition, there's no
> > > indication that this message requires a connected vdagent.
> >
> > How does the "need the agent connected" mechanism work? From what I
> > can
> > see, the check is at the beginning of
> > spice_main_channel_send_monitor_config(...) (spice-gtk, channel-
> > main.c):
> >
> > g_return_val_if_fail(c->agent_connected, FALSE);
> >
> > Which works the same with the new message. Is there more to it?
> >
> > > Also, per the spice protocol, vdagent messages are rate-limited by
> > > using a 'token' system. spice-gtk automatically handles that for us
> > > (and I think it also has some handling for vdagent messages that
> > > are
> > > attempted to be send when the vdagent is not yet connected). But by
> > > promoting this message to a MainChannel message, we've sidestepped
> > > these things.
> >
> > I haven't found anything in the code of spice-gtk that would handle
> > the
> > agent not connected state in a generic way. There's just the explicit
> > check mentioned above...
>
>
> What I was referring to is that while the agent is not connected, agent
> messages can be queued to be sent when the agent becomes connected. For
> example, agent_msg_queue_many() appends new agent messages to
> SpiceMainChannelPrivate::agent_msg_queue and these messages are sent
> out in agent_send_msg_queue() when the agent becomes connected and the
> client has tokens.
Ah, I missed the detail it won't send anything until it has tokens. It
seemed to me there was no agent connected check.
> *HOWEVER*, it appears that the old implementation also used the
> explicit agent_connected check that you mention above, and it avoided
> queueing monitors-config messages when the agent was not connected. So
> it appears that I was a bit wrong in my analysis and there's not
> actually a difference in behavior here when sending messages while the
> agent is not connected.
Seems so. I'm trying to figure out what would happen if the check
wasn't there and I don't see how it could block the main channel though
(although it's easy to miss something here). There is now a GQueue for
the outgoing agent messages on the server, which doesn't seems to be
limited, so the only thing preventing it going infinite is the token
mechanism? Maybe there was something different before introducing glib,
did it block the main channel though? (maybe move this part of
dicussion elsewhere, going offtopic)
> > For the token rate-limiting, first, what is the purpose of that?
> >
> > Second, is it important for this case? I've looked at the code, but
> > not
> > knowing the resons for it, I can't form an opinion about it...
>
> From docs/Spice_protocol.odt (quite outdated, but useful to understand
> the original protocol):
>
> A bidirectional tokens mechanism is used in order to prevent
> blocking of the main channel with agent messages (e.g., in case the
> agent stops consuming the data). Each side is not allowed to send
> more messages than the tokens allocated to it by the other side. The
> number of tokens that are allocated for the client is initialized
> from RED_MAIN_INIT message, and farther allocation of tokens is done
> using RED_MAIN_AGENT_TOKEN. Server tokens initial count is delivered
> in REDC_MAIN_AGENT_START message. This message must be the first
> agent related message that the client sends to the server. Farther
> tokens allocation for the server is done
> using REDC_MAIN_AGENT_TOKEN. Actual data packets are delivered
> using RED_MAIN_AGENT_DATA and REDC_MAIN_AGENT_DATA.
Makes me wonder... I don't see any dropping of the agent messages on
the client, so in case the amount of messages grows faster than the
number of tokens, the messages will get increasingly delayed? Probably
diverging again, but I wanted to understand it and I'm failing :)
> > > All of those things make me think that this may not be the best
> > > approach.
> >
> > Any ideas of a better one? :)
>
> well... As I mentioned in the patch 04/20, If this part of the protocol
> change is not strictly necessary, maybe we just drop it?
We can drop 04/20, but not this. The difference would be we would
translate this message to VDAgentMonitorsConfig instead of
VDAgentMonitorsConfigV2. The processing (i.e. changing for the ID pair
to guest_output_id) needs to stay...
> > By the way, similar thing happes for some mouse input messages sent
> > over the input channel, the server transforms them into VDAgent
> > messages, although only for client-side mouse mode.
>
> Hmm, interesting point. I hadn't thought about that case. That seems
> slightly problematic as well...
>
> >
> > > [*] of course, the server has had to do some processing of the
> > > monitors
> > > config message for a while now (to decide whether to pass the
> > > message
> > > to the agent or to the QXL device), but now the server will need to
> > > actually inspect and modify the data that will be sent down to the
> > > agent.
> > >
> > >
> > > On Thu, 2018-08-16 at 18:26 +0200, Lukáš Hrázký wrote:
> > > > The message is based on the VDAgentMonitorsConfig message already
> > > > being
> > > > sent from the client (but keeping the naming conventions of the
> > > > SpiceMsgDisplayMonitorsConfig, that is being sent from the client
> > > > to
> > > > the
> > > > server on the display channel), but adds the channel_id and
> > > > monitor_id
> > > > unique identification pair.
> > > >
> > > > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > > > ---
> > > > common/messages.h | 16 ++++++++++++++++
> > > > spice.proto | 14 ++++++++++++++
> > > > 2 files changed, 30 insertions(+)
> > > >
> > > > diff --git a/common/messages.h b/common/messages.h
> > > > index 55de76e..942ba07 100644
> > > > --- a/common/messages.h
> > > > +++ b/common/messages.h
> > > > @@ -112,6 +112,22 @@ typedef struct
> > > > SpiceMsgMainMigrationSwitchHost {
> > > > uint8_t *cert_subject_data;
> > > > } SpiceMsgMainMigrationSwitchHost;
> > > >
> > > > +typedef struct SpiceMsgcMainHead {
> > > > + uint32_t channel_id;
> > > > + uint32_t monitor_id;
> > > > + uint32_t width;
> > > > + uint32_t height;
> > > > + uint32_t depth;
> > > > + uint32_t x;
> > > > + uint32_t y;
> > > > +} SpiceMainHead;
> > > > +
> > > > +typedef struct SpiceMsgcMainMonitorsConfig {
> > > > + uint16_t count;
> > > > + uint32_t flags;
> > > > + SpiceMainHead heads[0];
> > > > +} SpiceMsgcMainMonitorsConfig;
> > > > +
> > > >
> > > > typedef struct SpiceMsgMigrate {
> > > > uint32_t flags;
> > > > diff --git a/spice.proto b/spice.proto
> > > > index 07f11ec..80976d4 100644
> > > > --- a/spice.proto
> > > > +++ b/spice.proto
> > > > @@ -340,6 +340,20 @@ channel MainChannel : BaseChannel {
> > > > } migrate_dst_do_seamless;
> > > >
> > > > Empty migrate_connected_seamless;
> > > > +
> > > > + message {
> > > > + uint16 count;
> > > > + uint32 flags;
> > > > + struct MainHead {
> > > > + uint32 channel_id;
> > > > + uint32 monitor_id;
> > > > + uint32 width;
> > > > + uint32 height;
> > > > + uint32 depth;
> > > > + uint32 x;
> > > > + uint32 y;
> > > > + } heads[count] @end;
> > > > + } monitors_config;
> > > > };
> > > >
> > > > enum8 clip_type {
More information about the Spice-devel
mailing list