Re: [spice-gtk v4 15/24] file-xfer: call user callback once per operation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2016-06-23 at 19:37 +0200, Victor Toso wrote:
> SpiceFileTransferTask has a callback to be called when operation
> ended. Til this patch, we were setting the user callback which means
> that in multiple file-transfers, we were calling the user callback
> several times.
> 
> Following the same logic pointed from 113093dd00a1cf10f6d3c3589b7 this
> is a SpiceMainChannel operation and it should only call the user
> callback when this operation is over (FileTransferOperation now).
> 
> Highlights:
> * Now using GTask to set user callback and callback_data
> * Handling error on FileTransferOperation:
> - It is considered an error if no file was successfully transferred due
>   cancellations or any other kind of error;
> - If any file failed due any error, we will consider the operation a
>   failure even if other files succeed.
> 
> Note also that SpiceFileTransferTask now does not have a callback and
> callback_data. The xfer_tasks has ended after its "finish" signal is
> emitted.
> ---
>  src/channel-main.c | 90 ++++++++++++++++++++++++++++++-----------------------
> -
>  1 file changed, 50 insertions(+), 40 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index fba5b7f..0505687 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -60,9 +60,7 @@ static GCancellable
> *spice_file_transfer_task_get_cancellable(SpiceFileTransferT
>  static GHashTable *spice_file_transfer_task_create_tasks(GFile **files,
>                                                           SpiceMainChannel
> *channel,
>                                                           GFileCopyFlags
> flags,
> -                                                         GCancellable
> *cancellable,
> -                                                         GAsyncReadyCallback
> callback,
> -                                                         gpointer user_data);
> +                                                         GCancellable
> *cancellable);
>  static void spice_file_transfer_task_init_task_async(SpiceFileTransferTask
> *self,
>                                                       GAsyncReadyCallback
> callback,
>                                                       gpointer userdata);
> @@ -162,9 +160,14 @@ typedef struct {
>      SpiceMainChannel           *channel;
>      GFileProgressCallback       progress_callback;
>      gpointer                    progress_callback_data;
> +    GTask                      *task;
>      struct {
>          goffset                 total_sent;
>          goffset                 transfer_size;
> +        guint                   num_files;
> +        guint                   succeed;
> +        guint                   cancelled;
> +        guint                   failed;
>      } stats;
>  } FileTransferOperation;
>  
> @@ -1841,7 +1844,6 @@ static void file_xfer_close_cb(GObject      *object,
>                                 GAsyncResult *close_res,
>                                 gpointer      user_data)
>  {
> -    GTask *task;
>      SpiceFileTransferTask *self;
>      GError *error = NULL;
>  
> @@ -1857,35 +1859,21 @@ static void file_xfer_close_cb(GObject      *object,
>          }
>      }
>  
> -    /* Notify to user that files have been transferred or something error
> -       happened. */
> -    task = g_task_new(self->channel,
> -                      self->cancellable,
> -                      self->callback,
> -                      self->user_data);
> -
> -    if (self->error) {
> -        g_task_return_error(task, self->error);
> -    } else {
> -        g_task_return_boolean(task, TRUE);
> -        if (spice_util_get_debug()) {
> -            gint64 now = g_get_monotonic_time();
> -            gchar *basename = g_file_get_basename(self->file);
> -            double seconds = (double) (now - self->start_time) /
> G_TIME_SPAN_SECOND;
> -            gchar *file_size_str = g_format_size(self->file_size);
> -            gchar *transfer_speed_str = g_format_size(self->file_size /
> seconds);
> +    if (self->error == NULL && spice_util_get_debug()) {
> +        gint64 now = g_get_monotonic_time();
> +        gchar *basename = g_file_get_basename(self->file);
> +        double seconds = (double) (now - self->start_time) /
> G_TIME_SPAN_SECOND;
> +        gchar *file_size_str = g_format_size(self->file_size);
> +        gchar *transfer_speed_str = g_format_size(self->file_size / seconds);
>  
> -            g_warn_if_fail(self->read_bytes == self->file_size);
> -            SPICE_DEBUG("transferred file %s of %s size in %.1f seconds
> (%s/s)",
> -                        basename, file_size_str, seconds,
> transfer_speed_str);
> +        g_warn_if_fail(self->read_bytes == self->file_size);
> +        SPICE_DEBUG("transferred file %s of %s size in %.1f seconds (%s/s)",
> +                    basename, file_size_str, seconds, transfer_speed_str);
>  
> -            g_free(basename);
> -            g_free(file_size_str);
> -            g_free(transfer_speed_str);
> -        }
> +        g_free(basename);
> +        g_free(file_size_str);
> +        g_free(transfer_speed_str);
>      }
> -    g_object_unref(task);
> -
>      g_object_unref(self);
>  }
>  
> @@ -3041,10 +3029,25 @@ failed:
>  static void file_transfer_operation_end(FileTransferOperation *xfer_op)
>  {
>      g_return_if_fail(xfer_op != NULL);
> -    spice_debug("Freeing file-transfer-operation %p", xfer_op);
> +
> +    if (xfer_op->stats.failed != 0 || xfer_op->stats.succeed == 0) {

So, if we have a 3-file transfer operation and two of them are cancelled, we
consider that a success. But if all three are cancelled, we consider that an
error? I can see the argument for that behavior, but how about something like
this instead?

if (failed != 0)
  return error (SPICE_CLIENT_ERROR_FAILED)
else if (cancelled != 0 && succeed == 0)
  return error (G_IO_ERROR_CANCELLED)
else
  return boolean (TRUE)


> +        GError *error = g_error_new(SPICE_CLIENT_ERROR,
> +                                    SPICE_CLIENT_ERROR_FAILED,
> +                                    "From %u files: %u succeed, %u cancelled,
> %u failed",
> +                                    xfer_op->stats.num_files,
> +                                    xfer_op->stats.succeed,
> +                                    xfer_op->stats.cancelled,
> +                                    xfer_op->stats.failed);
> +        SPICE_DEBUG("Transfer failed (%p) %s", xfer_op, error->message);
> +        g_task_return_error(xfer_op->task, error);
> +    } else {
> +        g_task_return_boolean(xfer_op->task, TRUE);
> +    }
>  
>      /* SpiceFileTransferTask itself is freed after it emits "finish" */
>      g_hash_table_unref(xfer_op->xfer_task_ht);
> +
> +    spice_debug("Freeing file-transfer-operation %p", xfer_op);
>      g_free(xfer_op);
>  }
>  
> @@ -3114,6 +3117,13 @@ static void
> file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
>      if (error) {
>          xfer_op->stats.total_sent -=
> spice_file_transfer_task_get_bytes_read(xfer_task);
>          xfer_op->stats.transfer_size -=
> spice_file_transfer_task_get_file_size(xfer_task);
> +        if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
> +            xfer_op->stats.cancelled++;
> +        } else {
> +            xfer_op->stats.failed++;
> +        }
> +    } else {
> +        xfer_op->stats.succeed++;
>      }
>  
>      /* Remove and free SpiceFileTransferTask */
> @@ -3179,7 +3189,11 @@ static SpiceFileTransferTask
> *spice_file_transfer_task_new(SpiceMainChannel *cha
>   * files, please connect to the #SpiceMainChannel::new-file-transfer signal.
>   *
>   * When the operation is finished, callback will be called. You can then call
> - * spice_main_file_copy_finish() to get the result of the operation.
> + * spice_main_file_copy_finish() to get the result of the operation. Note
> that
> + * before release 0.33 the callback was called for each file in multiple file
> + * transfer. This behavior was changed for the same reason as the
> + * progress_callback (above). If you need to monitor the ending of individual
> + * files, you can connect to "finished" signal from each
> SpiceFileTransferTask.
>   *
>   **/
>  void spice_main_file_copy_async(SpiceMainChannel *channel,
> @@ -3216,12 +3230,12 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
>      xfer_op->channel = channel;
>      xfer_op->progress_callback = progress_callback;
>      xfer_op->progress_callback_data = progress_callback_data;
> +    xfer_op->task = g_task_new(channel, cancellable, callback, user_data);
>      xfer_op->xfer_task_ht = spice_file_transfer_task_create_tasks(sources,
>                                                                    channel,
>                                                                    flags,
> -                                                                  cancellable
> ,
> -                                                                  callback,
> -                                                                  user_data);
> +                                                                  cancellable
> );
> +    xfer_op->stats.num_files = g_hash_table_size(xfer_op->xfer_task_ht);
>      g_hash_table_iter_init(&iter, xfer_op->xfer_task_ht);
>      while (g_hash_table_iter_next(&iter, &key, &value)) {
>          guint32 task_id;
> @@ -3303,9 +3317,7 @@ static guint64
> spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask *se
>  static GHashTable *spice_file_transfer_task_create_tasks(GFile **files,
>                                                           SpiceMainChannel
> *channel,
>                                                           GFileCopyFlags
> flags,
> -                                                         GCancellable
> *cancellable,
> -                                                         GAsyncReadyCallback
> callback,
> -                                                         gpointer user_data)
> +                                                         GCancellable
> *cancellable)
>  {
>      GHashTable *xfer_ht;
>      gint i;
> @@ -3326,8 +3338,6 @@ static GHashTable
> *spice_file_transfer_task_create_tasks(GFile **files,
>  
>          xfer_task = spice_file_transfer_task_new(channel, files[i],
> task_cancellable);
>          xfer_task->flags = flags;
> -        xfer_task->callback = callback;
> -        xfer_task->user_data = user_data;
>  
>          task_id = spice_file_transfer_task_get_id(xfer_task);
>          g_hash_table_insert(xfer_ht, GUINT_TO_POINTER(task_id), xfer_task);


Yet another question above, but otherwise looks good

Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]