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

[Jonathon Jongsma]

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

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