[Spice-devel] [spice-gtk v1] file-xfer: do not send unnecessary 0 bytes messages

[Victor Toso]

Hi,

On Fri, Nov 11, 2016 at 04:05:21PM -0600, Jonathon Jongsma wrote:
> On Fri, 2016-11-11 at 14:27 +0100, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> > 
> > This fixes the situation when VDAgent receives 0 byte message
> > regarding a file-transfer operation that was terminated in the
> > previous message.
> > 
> > This makes the VDAgent to send a STATUS_ERROR after STATUS_SUCCESS to
> > client.
> > 
> > Resolves: https://bugs.freedesktop.org/show_bug.cgi?id=97227
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/channel-main.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 990a06a..72ca712 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -1822,6 +1822,14 @@ static void file_xfer_read_async_cb(GObject
> > *source_object,
> >          return;
> >      }
> >  
> > +    if (count == 0 &&
> > spice_file_transfer_task_get_total_bytes(xfer_task) > 0) {
> > +        /* If we have sent all payload to the agent, we should not
> > send 0 bytes
> > +         * as it will cause https://bugs.freedesktop.org/show_bug.cg
> > i?id=97227.
> > +         * Only when file has 0 bytes of size is when we should send
> > 0 bytes to
> > +         * agent, see: https://bugzilla.redhat.com/show_bug.cgi?id=1
> > 135099 */
> > +        return;
> > +    }
> > +
> >      file_xfer_queue_msg_to_agent(channel,
> > spice_file_transfer_task_get_id(xfer_task), buffer, count);
> > 
> >      if (count == 0 ||
> > spice_file_transfer_task_is_completed(xfer_task)) {
> >          /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent
>
> Interesting. I see now though that there's a corner case that could
> still be slightly problematic:
>
> For a zero-length file, we do a read and then this callback is
> triggered with count == 0. We send that (empty) message to the agent.
> But say the agent doesn't send back its SUCCESS status very quickly and
> so the _data_flushed_cb() function intiates another async read.

Ah, but file_xfer_flush_async() is never called on zero-length files, as
we have:

 if (count == 0 || spice_file_transfer_task_is_completed(xfer_task)) {
     /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent
      * in case the task was completed, nothing to do. */
     return;
 }

> When
> the async read callback is triggered again, we get another zero-length
> read. Since the file size is zero, we pass the above conditions and
> send out a second zero-length data message to the agent. That's the
> same problem you're trying to fix above for non-zero-length files.

Yeah, it would be. I tried tranfering 10+ zero-length files to windows
and linux guests to see if something weird would be seen but no.

> It's
> clearly not as important because it's not at all common to transfer
> zero-length files, but it's probably still good to handle it.
>
> Another semi-related issue: what do we do if a buggy agent never sends
> a status response? Should we set a timer to cancel the transfer within
> a certain time after we've sent our last data message if we haven't
> received a response from the agent yet? Obviously that's not a issue to
> be solved in this patch, but it's somethign that I was wondering while
> reviewing the code.

A timer is simple and would work. We could also clear some data after
sending all the payload as we will not use it anymore... and trigger
some warnings on termination if we never had the SUCCESS/ERROR message
by them.

>
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>

Let me know if you think there is still an issue to be solved and many
thanks for the review!

Cheers,
  toso

>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161111/5ab180f4/attachment.sig>