[Spice-devel] [RFC PATCH spice-protocol 1/3] Add a graphics device info message

[Jonathon Jongsma]

On Wed, 2018-11-07 at 18:23 +0100, Lukáš Hrázký wrote:
> The message serves for passing the device address and device display
> ID

"serves for passing..." is a little strange to my ear. "serves to
pass..." sounds OK. Or "is used for passing..." 

> information for all display channels from SPICE server to the
> vd_agent.
> 
> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> ---
>  spice/vd_agent.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> index dda7044..a35f827 100644
> --- a/spice/vd_agent.h
> +++ b/spice/vd_agent.h
> @@ -91,6 +91,7 @@ enum {
>      VD_AGENT_CLIENT_DISCONNECTED,
>      VD_AGENT_MAX_CLIPBOARD,
>      VD_AGENT_AUDIO_VOLUME_SYNC,
> +    VD_AGENT_GRAPHICS_DEVICE_INFO,
>      VD_AGENT_END_MESSAGE,
>  };
>  
> @@ -248,6 +249,19 @@ typedef struct SPICE_ATTR_PACKED
> VDAgentAudioVolumeSync {
>      uint16_t volume[0];
>  } VDAgentAudioVolumeSync;
>  
> +typedef struct SPICE_ATTR_PACKED  {
> +    uint32_t channel_id;
> +    uint32_t monitor_id;
> +    uint32_t device_display_id;
> +    uint32_t device_address_len;
> +    uint8_t device_address[0];  // a zero-terminated string
> +} VDAgentGraphicsDeviceInfo;
> +
> +typedef struct SPICE_ATTR_PACKED VDAgentGraphicsDeviceInfos {
> +    uint32_t count;
> +    VDAgentGraphicsDeviceInfo graphics_device_infos[0];
> +} VDAgentGraphicsDeviceInfos;
> +

I'm not a big fan of this name. "Information" is slightly irregular
word which doesn't really have singular and plural forms. So
"Informations" or "Infos" sounds wrong to me. 

In addition, the VDAgentGraphicsDeviceInfo name implies to me that this
struct represents information about a single device. But in reality, it
represents information about a single spice display -- specifically
which device display is associated with the spice display. So a single
device with multiple displays would be represented by multiple info
structs. So maybe a better name might be... (just brainstorming here):
VDAgentSpiceDisplayInfo
VDAgentChannelDisplayInfo
VDAgentSpiceDisplayDeviceInfo

None of them are very nice, but I feel like the 'focus' of the name
should be the on the spice display rather than the device...

Also, I was comparing this message to the monitors config message and
noticed that that message has a 'flags' field to accomodate different
behavior that was added over time. If we add a 'flags' field, it would
give us some future-proofing that may allow us to interpret the data
slightly differently based on the existence of different flags.
Worthwhile?


>  enum {
>      VD_AGENT_CAP_MOUSE_STATE = 0,
>      VD_AGENT_CAP_MONITORS_CONFIG,
> @@ -264,6 +278,7 @@ enum {
>      VD_AGENT_CAP_MONITORS_CONFIG_POSITION,
>      VD_AGENT_CAP_FILE_XFER_DISABLED,
>      VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> +    VD_AGENT_CAP_GRAPHICS_DEVICE_INFO,
>      VD_AGENT_END_CAP,
>  };
>