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

Submitted by Victor Toso on Nov. 11, 2016, 1:27 p.m.

Details

Message ID 20161111132734.2000-1-victortoso@redhat.com
State Accepted
Commit 885229393e3198bd4b386cf71a51832439dfb31b
Headers show
Series "file-xfer: do not send unnecessary 0 bytes messages" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Nov. 11, 2016, 1:27 p.m.
From: Victor Toso <me@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@redhat.com>
---
 src/channel-main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Patch hide | download patch | download mbox

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.cgi?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=1135099 */
+        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

Comments

On Fri, 2016-11-11 at 14:27 +0100, Victor Toso wrote:
> From: Victor Toso <me@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@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@redhat.com>
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@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@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
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@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@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@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@redhat.com>
Hi,

On Mon, Nov 14, 2016 at 12:33:46PM -0600, Jonathon Jongsma wrote:
> 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. 

True!

On spice_main_channel_reset(), we would free the queue to the agent with
agent_free_msg_queue() and the message might not be sent and we would
not have any error/warning.

Unlikely but it seems possible... Maybe on migration?

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

Many thanks! I'm always learning with your reviews.

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

Pushed as 885229393e3198bd4b386cf71a51832439dfb31b

Cheers,
  toso