[Spice-devel] [RFC PATCH spice-common v2 01/20] Add a monitors config message from the client to the server

[Lukáš Hrázký]

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 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 {