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

[Jonathon Jongsma]

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>


>  
>      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);