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

[Pavel Grunt]

On Wed, 2017-05-17 at 07:17 -0400, Frediano Ziglio wrote:
> > 
> > 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.

it would have to be considered when implementing non error results,
the cap is for errors. The agent fallbacks to a general
VD_AGENT_FILE_XFER_STATUS_ERROR  if the client does not have the cap.


> Maybe we can use a some bits in the status (like bit 31) to mark
> proper additional
> error?

imho instead of mixing result types and some flags is better to have a
 new message type


> I assume an agent supporting VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS
> shold accept
> additional data appended after VDAgentFileXferStatusMessage.

In the current implementation the cap is only for the client. Client 
may expect additional data (bigger message size) if it has the flag.

Thanks,
Pavel

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