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

[Jonathon Jongsma]

On Mon, 2018-11-12 at 11:01 +0100, Lukáš Hrázký wrote:
> 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".

Right, except the data we're passing is not exactly the same thing that
qemu passed to us. Qemu passed us a device address and a list of
displays associated with a QXL instance. We took that information and
essentially created a table which maps from a spice protocol display id
to some information that describes a guest device display. That
*mapping* is the important concept that we're passing down to the
guest. After all, the device information is already essentially known
to the guest. So it feels slightly misleading to call it
VDAgentGraphicsDeviceInfo. Maybe something like
VDAgentDeviceDisplayMapping??

Note that I'm not trying to make a big deal of this. If we can't come
up with a better name, I'm OK with the original one.

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


yeah, I was mostly pondering aloud. I'm not opposed to moving forward
without.



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