[Spice-devel] [PATCH spice-protocol v2] agent: Add support for reporting on free space

[Frediano Ziglio]

> 
> Hi Jakub,
> 
> On Tue, 2017-05-09 at 18:53 +0200, Jakub Janků wrote:
> > Agent can send VDAgentFileXferStatusMessage with result
> > VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE to indicate lack of free
> > space. This enables more detailed error reporting, so the user knows
> > why the file transfer has failed.
> > 
> > Add VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS to ensure detailed errors
> > are not sent to clients that do not support it. This can be used
> > with more file xfer errors in the future.
> > ---
> >  spice/vd_agent.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > index 3b1f183..d2477ce 100644
> > --- a/spice/vd_agent.h
> > +++ b/spice/vd_agent.h
> > @@ -99,11 +99,16 @@ enum {
> >      VD_AGENT_FILE_XFER_STATUS_CANCELLED,
> >      VD_AGENT_FILE_XFER_STATUS_ERROR,
> >      VD_AGENT_FILE_XFER_STATUS_SUCCESS,
> > +    VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE,
> >  };
> >  
> >  typedef struct SPICE_ATTR_PACKED VDAgentFileXferStatusMessage {
> >     uint32_t id;
> >     uint32_t result;
> > +   /* Used to send additional data for detailed error messages
> > +    * to clients with VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS
> > capability.
> > +    */
> 
> the comment should be expanded to say the data type for the particular
> status result. eg VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE uint64
> 
> > +   uint8_t data[0];
> >  } VDAgentFileXferStatusMessage;
> >  
> >  typedef struct SPICE_ATTR_PACKED VDAgentFileXferStartMessage {
> > @@ -248,6 +253,7 @@ enum {
> >      VD_AGENT_CAP_AUDIO_VOLUME_SYNC,
> >      VD_AGENT_CAP_MONITORS_CONFIG_POSITION,
> >      VD_AGENT_CAP_FILE_XFER_DISABLED,
> > +    VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> >      VD_AGENT_END_CAP,
> >  };
> >  

Is this capability enough?
I means, this basically declare that the client wants any additional
details. What's happen in the future if another VD_AGENT_FILE_XFER_STATUS_xxx
is added? Should the agent just consider the additional constants as error and
ignore the attached data? What if in the future there's a status like SUCCESS
and CANCELLED which is not exactly an error? Will be handled as error.
Maybe we can use a some bits in the status (like bit 31) to mark proper additional
error?
I assume an agent supporting VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS shold accept
additional data appended after VDAgentFileXferStatusMessage.

> 
> besides that it looks good to me
> 
> Pavel
> 

Frediano