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.
>
> > 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.
*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.
>
> 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.
>
> > 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?
>
> 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@xxxxxxxxxx>
> > > ---
> > > 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 {
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel