[Spice-devel] [PATCH spice 3/3] reds: Cancel unexpected xfer messages

[Pavel Grunt]

On Fri, 2017-06-02 at 04:13 -0400, Frediano Ziglio wrote:
> Not much related to other patches

a bit, by location :) I can send it separately, I wanted to avoid
merge conflicts.

> 
> > 
> > Inform client that transfer will not work, instead of only
> > dropping the agent messages.
> > 
> > The file transfer messages can be disabled using qemu cli option:
> >  "disable-agent-file-xfer"
> > 
> > Resolves:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1373725
> > 
> > Signed-off-by: Pavel Grunt <pgrunt at redhat.com>
> > ---
> > Can be extended to use a detailed status message introduced
> > recently in
> > protocol
> > ---
> >  server/reds.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 8abeb3fbd..38d34c827 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -1110,6 +1110,39 @@ static void
> > reds_on_main_agent_monitors_config(RedsState *reds, const void
> > *mess
> >      reds_client_monitors_config_cleanup(reds);
> >  }
> >  
> > +static void agent_data_free(uint8_t *data, void *opaque
> > G_GNUC_UNUSED)
> > +{
> > +    free(data);
> > +}
> > +
> > +/*
> > + * Inform client about discarded file transfer
> > + * https://bugzilla.redhat.com/show_bug.cgi?id=1373725
> > + */
> > +static void agent_xfer_error(MainChannelClient *mcc, const
> > VDAgentMessage
> > *message)
> > +{
> > +    VDAgentMessage *xfer_msg;
> > +    VDAgentFileXferStatusMessage *xfer_status;
> > +    size_t len;
> > +
> > +    if (message->type < VD_AGENT_FILE_XFER_START || message->type 
> > >
> > VD_AGENT_FILE_XFER_DATA) {
> > +        return;
> > +    }
> > +
> > +    len = sizeof(VDAgentMessage) +
> > sizeof(VDAgentFileXferStatusMessage);
> > +    xfer_msg = spice_malloc0(len);
> > +    xfer_msg->protocol = VD_AGENT_PROTOCOL;
> > +    xfer_msg->type = VD_AGENT_FILE_XFER_STATUS;
> > +    xfer_msg->size = sizeof(VDAgentFileXferStatusMessage);
> > +    xfer_status = (VDAgentFileXferStatusMessage *) xfer_msg-
> > >data;
> > +    xfer_status->id = *((uint32_t *) message->data);
> 
> This is not really portable. See also patches from Christophe D
> for clang.
> 
> I would personally define some integers like (not tested..
> just did some test with smaller definitions)
> 
> 
> #include <spice/start-packed.h>
> 
> #define PACKET_INT_DEFINITION(type) \
> typedef struct SPICE_ATTR_PACKED type ## _packed_t { \
>     type ## _t value; \
> } type ## _packed_t
> 
> PACKET_INT_DEFINITION(int8);
> PACKET_INT_DEFINITION(int16);
> PACKET_INT_DEFINITION(int32);
> PACKET_INT_DEFINITION(int64);
> 
> PACKET_INT_DEFINITION(uint8);
> PACKET_INT_DEFINITION(uint16);
> PACKET_INT_DEFINITION(uint32);
> PACKET_INT_DEFINITION(uint64);
> 
> #undef PACKET_INT_DEFINITION
> 
> #include <spice/end-packed.h>
> 
> 
> and write something like
> 
>    xfer_status->id = ((uint32_packed_t *) message->data)->value;
> 
> unfortunately the structure is required as packed attribute
> applied to a pointer is useless (gcc just ignore it).
> But using this code the compiler is able to generate good code
> for whatever platform (tested on sparc and other architecture
> which are not really unaligned friendly).
> 
> Maybe TYPE_unaligned_t is a better name?
> 
> In this case also a
> 
>    xfer_status->id = ((VDAgentFileXferStartMessage *) message-
> >data)->id;
> 
> would be fine

I'd go for this

> 
> > +    xfer_status->result = VD_AGENT_FILE_XFER_STATUS_ERROR;
> > +
> > +    spice_debug("canceling file transfer %u", xfer_status->id);
> > +
> > +    main_channel_client_push_agent_data(mcc, (uint8_t *)
> > xfer_msg, len,
> > agent_data_free, NULL);
> > +}
> > +
> >  void reds_on_main_agent_data(RedsState *reds, MainChannelClient
> > *mcc, const
> >  void *message,
> >                               size_t size)
> >  {
> > @@ -1123,6 +1156,7 @@ void reds_on_main_agent_data(RedsState
> > *reds,
> > MainChannelClient *mcc, const void
> >      case AGENT_MSG_FILTER_OK:
> >          break;
> >      case AGENT_MSG_FILTER_DISCARD:
> > +        agent_xfer_error(mcc, message);
> >          return;
> >      case AGENT_MSG_FILTER_MONITORS_CONFIG:
> >          reds_on_main_agent_monitors_config(reds, message, size);
> 
> Not tested, not sure what happen for old clients and if is correct
> to

cannot break the old client or new clients. they are ready to receive
the status error

client should send the "transfer start message" and wait for the "can
send data"

> send back an error for every discarded data.
I can store an id of transfer somewhere a avoid discarding the same
transfer more than once. imo it is overkill, client must survive xfer
error status.

>  Maybe the client will continue
> to send data (for instance for a file transfer)

It should not, once it gets the error status it cancels the transfer

Thanks,
Pavel

>  generating lot of go and
> back?
> 
> Frediano