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

[Lukáš Hrázký]

On Thu, 2018-11-08 at 11:36 -0600, Jonathon Jongsma wrote:
> 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..." 

Ok, thanks :)

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

Yes, I know it's incorrect in english. I just followed the same
convention I always use, name a struct and then use the plural to name
the container for it... What am I supposed to do if it doesn't work for
"Info"? :) I need to name the inner struct and then the one containing
the list, I prefer this to some arbitrary plays on the name...

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

What I did was take the name we used for the QEMU interface method:
spice_qxl_set_device_info. I thought "device_info" is too generic
outside a QXL interface context so I added the "graphics" to the
"device".

I don't find your suggestions particularly better :) Especially those
containing the word "Spice", I think that's redundant?

The outer struct with the array kind of does contain all the device
info, while the inner one is per display/monitor, so what about:

struct VDAgentGraphicsDeviceInfo {
    ...
    VDAgentGraphicsDeviceDisplayInfo graphics_device_display_info[0];
};

We can still discuss the "GraphicsDevice" part (I'm not too happy about
it myself), but I still can't think of a better one...

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

I don't know. The flags add the possibility to pass boolean values,
which is a very limited set of cases you are preparing for. Since we
can't predict the future... I'd lean towards not adding it. We can also
get into the (non)extesibility debate and perhaps add some generic
"padding" to the structs which could be used for anything later, but...

Thanks,
Lukas

> >  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,
> >  };