[Spice-devel] [spice-gtk v5 14/23] file-xfer: call user callback once per operation

Victor Toso lists at victortoso.com
Thu Jul 7 13:38:47 UTC 2016


Hi,

On Wed, Jul 06, 2016 at 11:15:12AM -0500, Jonathon Jongsma wrote:
> On Tue, 2016-07-05 at 15:07 +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.
> > 
> > Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> > ---
> >  src/channel-main.c | 101 ++++++++++++++++++++++++++++++++------------------
> > ---
> >  1 file changed, 61 insertions(+), 40 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index c5540e7..fa5b611 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);
> > @@ -161,9 +159,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;
> >  
> > @@ -1840,7 +1843,6 @@ static void file_xfer_close_cb(GObject      *object,
> >                                 GAsyncResult *close_res,
> >                                 gpointer      user_data)
> >  {
> > -    GTask *task;
> >      SpiceFileTransferTask *self;
> >      GError *error = NULL;
> >  
> > @@ -1856,35 +1858,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);
> >  }
> >  
> > @@ -3051,10 +3039,36 @@ failed:
> >  static void file_transfer_operation_free(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) {
> > +        GError *error = g_error_new(SPICE_CLIENT_ERROR,
> > +                                    SPICE_CLIENT_ERROR_FAILED,
> > +                                    "From %u files: %u succeed, %u cancelled,
> > %u failed",
> 
> Missed this last time, but I think this error message may need some context. If
> the UI just presented this message, you might be able to connect it to a file
> transfer operation, but it's not perfectly clear. So perhaps change it to
> something like:
> 
> "Transferring %u files: %u succeed, %u cancelled, %u failed"?

Sure

> 
> 
> > +                                    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 if (xfer_op->stats.cancelled != 0 && xfer_op->stats.succeed == 0)
> > {
> > +        GError *error = g_error_new(G_IO_ERROR,
> > +                                    G_IO_ERROR_CANCELLED,
> > +                                    "From %u files: %u succeed, %u cancelled,
> > %u failed",
> 
> same here.

Ok

> 
> > +                                    xfer_op->stats.num_files,
> > +                                    xfer_op->stats.succeed,
> > +                                    xfer_op->stats.cancelled,
> > +                                    xfer_op->stats.failed);
> > +        SPICE_DEBUG("Transfer cancelled (%p) %s", xfer_op, error->message);
> > +        g_task_return_error(xfer_op->task, error);
> > +    } else {
> > +        SPICE_DEBUG("Transfer successful (%p)", xfer_op);
> > +        g_task_return_boolean(xfer_op->task, TRUE);
> > +    }
> >  
> >      /* SpiceFileTransferTask itself is freed after it emits "finish" */
> >      g_hash_table_unref(xfer_op->xfer_task);
> > +
> > +    spice_debug("Freeing file-transfer-operation %p", xfer_op);
> >      g_free(xfer_op);
> >  }
> >  
> > @@ -3130,6 +3144,13 @@ static void
> > file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
> >          guint64 file_size =
> > spice_file_transfer_task_get_file_size(xfer_task);
> >          guint64 bytes_read =
> > spice_file_transfer_task_get_bytes_read(xfer_task);
> >          xfer_op->stats.transfer_size -= (file_size - bytes_read);
> > +        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 */
> > @@ -3195,7 +3216,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,
> > @@ -3232,12 +3257,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 = 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);
> >      g_hash_table_iter_init(&iter, xfer_op->xfer_task);
> >      while (g_hash_table_iter_next(&iter, &key, &value)) {
> >          guint32 task_id;
> > @@ -3318,9 +3343,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;
> > @@ -3342,8 +3365,6 @@ static GHashTable
> > *spice_file_transfer_task_create_tasks(GFile **files,
> >          /* FIXME: Move the xfer-task initialization to
> > spice_file_transfer_task_new() */
> >          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);
> 
> Minor comment above.
> 
> Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

Thanks,
  toso
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


More information about the Spice-devel mailing list