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

[Christophe de Dinechin]

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

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).

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


> +    /* 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.

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).


> +    */
> +    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?

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

> +    /*
> +     * 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