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

[Jonathon Jongsma]

On Fri, 2016-11-11 at 23:53 +0100, Victor Toso wrote:
> 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_bu
> > > g.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;
>  }
> 

hmm, you're right. But it's a little bit weird that we don't flush this
message but we do all other file-transfer messages. That means that for
zero-length files we'll never receive GError reports for failed
sending/flushing of the message. This doesn't matter at the moment
since it doesn't look like the flush can actually fail. So I guess it
doesn't really matter. 


> > 
> > 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!

Not really anything additional needed for this patch. It just triggered
some thoughts about semi-related stuff.

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