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...
> 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...
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...
> All of those things make me think that this may not be the best
> approach.
Any ideas of a better one? :)
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.
> [*] 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