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

[Lukáš Hrázký]

On Mon, 2018-11-12 at 14:44 -0600, Jonathon Jongsma wrote:
> 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??

Hmm. One thing I was aiming for was keeping the name similar to what's
used in the QEMU interface, makes it easier for others reading the code
to associate I think.

You're right it is passing the mapping, but I think it is a common case
when you're sending some data that there are some IDs that go with them
and it's clear the data belong (map) to the IDs. But yeah, in this case
the data are used for identification in the guest, so calling it
mapping makes more sense.

Still, not sure how much it helps and keeping the name similar seems
more important to me.

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

As petty as these discussions seem to be, I think it's good to have
them... bad names are bad :)

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

So the above doesn't work for you? Is VDAgentDisplayDeviceInfo and
VDAgentDisplayDeviceDisplayInfo too much? :)

If we went with your "mapping" name, how would you name the inner
struct?

Cheers,
Lukas

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