[Spice-devel] [RFC/POC PATCH spice-protocol 02/16] Create a version 2 of the VDAgentMonitorsConfig message

Tue Jun 19 15:30:46 UTC 2018

On Tue, 2018-06-19 at 15:41 +0200, Christophe de Dinechin wrote:
> > On 5 Jun 2018, at 17:30, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> > 
> > To keep compatibility with old endpoints (any of client, server,
> > vd_agent), we need to copy the message to add the output_id field.
> > 
> > The output_id is the guest-side id of the xrandr output (to be precise,
> > it is the index in the list of xrandr outputs) that is set in the
> > monitors config messages by the streaming agent. It is later used in the
> > guest by vd_agent for mouse input and possibly monitors_config
> > (enabling/disabling monitors and setting the resolution/position of
> > monitors).
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> > spice/vd_agent.h | 27 +++++++++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> > 
> > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > index dda7044..05c9c40 100644
> > --- a/spice/vd_agent.h
> > +++ b/spice/vd_agent.h
> > @@ -154,6 +154,33 @@ typedef struct SPICE_ATTR_PACKED VDAgentMonitorsConfig {
> >     VDAgentMonConfig monitors[0];
> > } VDAgentMonitorsConfig;
> > 
> > +typedef struct SPICE_ATTR_PACKED VDAgentMonConfigV2 {
> 
> So you are defining a whole new structure just to add one field, right?

Right.

> Would it be better from a compatibility point of view to add a flag indicating, for example, that the “depth” field is replaced with:
> 
> 	uint16_t depth;
> 	uint16_t output_id;
> 
> and leave the output_id to 0 unless we have the capability you added? (I ordered the fields assuming little endian).

I don't know. I've also just noticed today that the depth field
actually seems to be unused, besides being set in the client. But the
fact that it is being set complicates reuse. And your solution would
have a problem on big endian architectures?

I'm not much fond of reusing current fields if it involves yet another
hack. I also was already planning to add the channel_id and monitor_id
to the message to fix the ID problem in general. So that probably
warrants a new message.

There is the "opposite" message (SpiceMsgDisplayMonitorsConfig) to
consider too, the one sent from the server to the client (patch 03/16,
needs to be copied too). That one has the "uint32_t flags" field that
seems completely unused. That may be a better candidate, since reusing
the field should be clean and there are no other IDs to add.

> Alternatively, if you want to add a new field, you might want to leave some room for future extensions.

Yeah, maybe... and how much? This triggered me thinking having a
protocol that is extensible (e.g. protocol buffers, cap'n proto) would
be much better (topic for another discussion though :D)

> > +    /* The output_id is the guest-side id of the xrandr output (to be precise,
> > +     * it is the index in the list of xrandr outputs) that is set in the
> > +     * monitors config messages by the streaming agent. It is later used in the
> > +     * guest by vd_agent for mouse input and possibly monitors_config
> > +     * (enabling/disabling monitors and setting the resolution/position of
> > +     * monitors).
> 
> Is it useful to explicitly link that to the xrandr output? As far as the protocol or client are concerned, it’s just some opaque output ID.

Not necessarily, but the link is only in the comment. That can be
changed anytime without compatibility reprecussions :)

> On the other hand, I would add the producers and consumers of this data, and then you could list xrandr as an example (as opposed to a definition).

I can do that. As I said, that's only said in comments and easily
changeable. I didn't want to draw some theoretical rules and boundaries
around the one number that are not going to matter much once someone
needs to change the content :)

Note there is the semantic of "0" representing an unset output_id,
which might be limiting to the possible usage...

I see you also already noted the "-1" I'm doing with this for the mouse
motion event... :D Not that great, I agree.

> > +    */
> > +    uint32_t output_id;
> 
> If it’s opaque, 32-bit might be too small, e.g. to pass a Windows handle. Or is it part of the definition that these are necessarily small consecutive IDs and that the agent has to keep a mapping if they want to associate some pointer with it?

I think you have the same information I do here :) The xrandr output
IDs are the only thing we have atm. I never finished the implementation
of my crystal ball so I can't predict the future yet :)

If the protocol was extensible, it wouldn't be much of an issue. The
way it is, I used uint32_t thinking it should be enough for any needs
we have in the future. So the windows handles are what, uint64_t? From
my very brief googling it seems there's some confusion around it, but
that handles seem to in general fit in 32 bytes...

> (Yes, I know it contradicts my compatible proposal above, just trying to confuse you, or more realistically, to understand what you have in mind ;-)

It's ok, this is the sort of discussion I wanted to have at this stage.

Thanks,
Lukas

> > +    /*
> > +     * Note a width and height of 0 can be used to indicate a disabled
> > +     * monitor, this may only be used with agents with the
> > +     * VD_AGENT_CAP_SPARSE_MONITORS_CONFIG capability.
> > +     */
> > +    uint32_t height;
> > +    uint32_t width;
> > +    uint32_t depth;
> > +    int32_t x;
> > +    int32_t y;
> > +} VDAgentMonConfigV2;
> > +
> > +typedef struct SPICE_ATTR_PACKED VDAgentMonitorsConfigV2 {
> > +    uint32_t num_of_monitors;
> > +    uint32_t flags;
> > +    VDAgentMonConfigV2 monitors[0];
> > +} VDAgentMonitorsConfigV2;
> > +
> > enum {
> >     VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_WALLPAPER = (1 << 0),
> >     VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_FONT_SMOOTH = (1 << 1),
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 