[Spice-devel] [spice-gtk v3 08/16] file-xfer: fix progress info on cancelled transfers

[Victor Toso]

Hi,

On Fri, Jun 03, 2016 at 12:22:09PM -0500, Jonathon Jongsma wrote:
> On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> > Application can start multiple file-transfers in one operation and
> > cancel a few of them while the operation is ongoing. In that case, we
> > should remove the file-size of the transfer operation otherwise we
> > will send incorrect progress data.
> > 
> > Taking in consideration the split of SpiceFileTransferTask, this patch
> > includes spice_file_transfer_task_get_file_size() internal helper
> > function.
> > ---
> >  src/channel-main.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index e204a1e..890d939 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -61,6 +61,7 @@ typedef void
> > (*SpiceFileTransferTaskFlushCb)(SpiceFileTransferTask *xfer_task,
> >  static guint32 spice_file_transfer_task_get_id(SpiceFileTransferTask *self);
> >  static SpiceMainChannel
> > *spice_file_transfer_task_get_channel(SpiceFileTransferTask *self);
> >  static GCancellable
> > *spice_file_transfer_task_get_cancellable(SpiceFileTransferTask *self);
> > +static guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> > *self);
> >  static void spice_file_transfer_task_flush_done(SpiceFileTransferTask *self,
> > GError *error);
> >  static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel
> > *channel,
> >                                                      GFile **files,
> > @@ -1927,6 +1928,7 @@ static void file_xfer_end_callback(GObject
> > *source_object,
> >  {
> >      GTask *task;
> >      FileTransferOperation *xfer_op;
> > +    SpiceFileTransferTask *xfer_task;
> >  
> >      task = G_TASK(res);
> >      if (!g_task_had_error(task))
> > @@ -1934,7 +1936,9 @@ static void file_xfer_end_callback(GObject
> > *source_object,
> >           * file_transfer_operation_task_finished */
> >          return;
> >  
> > +    xfer_task = SPICE_FILE_TRANSFER_TASK(source_object);
> >      xfer_op = user_data;
> > +    xfer_op->transfer_size -=
> > spice_file_transfer_task_get_file_size(xfer_task);
> 
> I don't think this is sufficient. The file might get cancelled after it has
> transferred some of its data already. In this case, that already-transferred
> data will have been added to total_sent, but we will subtract the file size from
> transfer_size. Consider the situation where you're transferring two 1MB files.
> If you cancel one of these files after sending 75% of both files, the next
> progress update will indicate that you've sent 1.5MB of 1MB (or 150%) even
> though you still have 25% of the other file to send yet.
>
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>

You are right!

I guess the correct would be by having "total_flushed" data in each
SpiceFileTransferTask and use that to subtract from total_sent.

I'll fix it, thanks!

> 
> 
> >  
> >      if (xfer_op->error != NULL)
> >          return;
> > @@ -1948,9 +1952,6 @@ static void file_xfer_end_callback(GObject
> > *source_object,
> >       * without GCancellabe */
> >      if (g_error_matches(xfer_op->error, G_IO_ERROR, G_IO_ERROR_CANCELLED) &&
> >              xfer_op->cancellable == NULL) {
> > -        SpiceFileTransferTask *xfer_task;
> > -
> > -        xfer_task = SPICE_FILE_TRANSFER_TASK(source_object);
> >          spice_debug("file-transfer %u was cancelled",
> >                      spice_file_transfer_task_get_id(xfer_task));
> >          g_clear_error(&xfer_op->error);
> > @@ -3382,6 +3383,12 @@ static GCancellable
> > *spice_file_transfer_task_get_cancellable(SpiceFileTransferT
> >      return self->cancellable;
> >  }
> >  
> > +static guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> > *self)
> > +{
> > +    g_return_val_if_fail(self != NULL, 0);
> > +    return self->file_size;
> > +}
> > +
> >  static void spice_file_transfer_task_flush_done(SpiceFileTransferTask *self,
> > GError *error)
> >  {
> >      g_return_if_fail(self != NULL);