[Spice-devel] [PATCH spice-protocol 1/8] Add the VDAgentGraphicsDeviceInfo message

[Lukáš Hrázký]

On Wed, 2019-01-09 at 06:30 -0500, Frediano Ziglio wrote:
> > 
> > On Wed, 2019-01-09 at 02:41 -0500, Frediano Ziglio wrote:
> > > > 
> > > > The message serves for passing the device address and device display ID
> > > > 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..5e618b7 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 VDAgentDeviceDisplayInfo {
> > > > +    uint32_t channel_id;
> > > > +    uint32_t monitor_id;
> > > 
> > > Other parts of the SPICE protocol use 8 bit for these.
> > > Do you want to extend them in the future?
> > > Maybe better to put some comment on possible limits?
> > 
> > channel_id is 8 bits on the client <-> server line, monitor_id is 32
> 
> Yes, confused for monitor_id.
> 
> > bits though. What comment on limits? You need to use existing valid
> > IDs, otherwise it won't work. I don't see any other limitation.
> > 
> > I can change them (or just channel_id) to 8 bits if you want... I'd
> > just leave them like this myself though.
> > 
> 
> or a comment like "// currently limited to 255".

Ok, but I find no value in a comment like this. The fact that a valid
ID will atm. never be greater than 255 does not really impose a limit
here. If you are putting an ID greater than 255 in here, you are
putting a non-existent ID, but that's all. There are always non-
existent IDs that will not work. No difference.

> > > > +    uint32_t device_display_id;
> > > > +    uint32_t device_address_len;
> > > > +    uint8_t device_address[0];  // a zero-terminated string
> > > 
> > > Similar comments for spice-protocol for these last 2 fields.
> > 
> > Not sure what you mean by this...
> > 
> 
> Comments on patch 2/8.

About zero-termination? The length is actually needed here.

> > Cheers,
> > Lukas
> > 
> > > > +} VDAgentDeviceDisplayInfo;
> > > > +
> > > > +typedef struct SPICE_ATTR_PACKED VDAgentGraphicsDeviceInfo {
> > > > +    uint32_t count;
> > > > +    VDAgentDeviceDisplayInfo device_info[0];
> > > > +} VDAgentGraphicsDeviceInfo;
> > > > +
> > > >  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,
> > > >  };
> > > >  
> > > 
> > > Frediano