Re: [spice-gtk v3 03/16] file-xfer: introduce create_tasks and start_task

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

 



On Mon, 2016-05-30 at 11:54 +0200, Victor Toso wrote:
> By splitting file_xfer_send_start_msg_async we can separate in three
> different steps the spice_main_file_copy_async function:
> 
> 1-) Creating tasks with spice_file_transfer_task_create_tasks which
>     now returns a GList of SpiceFileTransferTask;
> 2-) Setting handlers before the SpiceFileTransferTask starts;
> 3-) Starting the task with spice_file_transfer_task_start_task
> 
> (1) and (3) can be done outside channel-main scope.
> 
> This change is related to split SpiceFileTransferTask from
> channel-main.
> ---
>  src/channel-main.c | 150 +++++++++++++++++++++++++++++++---------------------
> -
>  1 file changed, 88 insertions(+), 62 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 702f146..7843aeb 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -62,6 +62,17 @@ 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 void spice_file_transfer_task_flush_done(SpiceFileTransferTask *self,
> GError *error);
> +static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel
> *channel,
> +                                                    GFile **files,
> +                                                    GFileCopyFlags flags,
> +                                                    GCancellable
> *cancellable,
> +                                                    GFileProgressCallback
> progress_callback,
> +                                                    gpointer
> progress_callback_data,
> +                                                    SpiceFileTransferTaskFlus
> hCb flush_callback,
> +                                                    gpointer
> flush_callback_data,
> +                                                    GAsyncReadyCallback
> callback,
> +                                                    gpointer user_data);

I think I would prefer to re-order the arguments of this function. Generally the
first argument of the function should be the most important one (e.g. the 'self'
argument to a class method). In this case, since the function creates multiple
tasks for a series of files, I think that perhaps the array of files should be
the first argument. Just a personal preference thing, but I don't care too much.


> +static void spice_file_transfer_task_start_task(SpiceFileTransferTask *self);
>  
>  /**
>   * SECTION:file-transfer-task
> @@ -3066,59 +3077,9 @@ static void task_finished(SpiceFileTransferTask *task,
>                            gpointer data)
>  {
>      SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
> -    g_hash_table_remove(channel->priv->file_xfer_tasks,
> GUINT_TO_POINTER(task->id));
> -}
> -
> -static void file_xfer_send_start_msg_async(SpiceMainChannel *channel,
> -                                           GFile **files,
> -                                           GFileCopyFlags flags,
> -                                           GCancellable *cancellable,
> -                                           GFileProgressCallback
> progress_callback,
> -                                           gpointer progress_callback_data,
> -                                           SpiceFileTransferTaskFlushCb
> flush_callback,
> -                                           gpointer flush_callback_data,
> -                                           GAsyncReadyCallback callback,
> -                                           gpointer user_data)
> -{
> -    SpiceMainChannelPrivate *c = channel->priv;
> -    SpiceFileTransferTask *task;
> -    gint i;
> -
> -    for (i = 0; files[i] != NULL && !g_cancellable_is_cancelled(cancellable);
> i++) {
> -        GCancellable *task_cancellable = cancellable;
> -        /* if a cancellable object was not provided for the overall
> operation,
> -         * create a separate object for each file so that they can be
> cancelled
> -         * separately  */
> -        if (!task_cancellable)
> -            task_cancellable = g_cancellable_new();
> +    guint32 task_id = spice_file_transfer_task_get_id(task);
>  
> -        task = spice_file_transfer_task_new(channel, files[i],
> task_cancellable);
> -        task->flags = flags;
> -        task->progress_callback = progress_callback;
> -        task->progress_callback_data = progress_callback_data;
> -        task->flush_callback = flush_callback;
> -        task->flush_callback_data = flush_callback_data;
> -        task->callback = callback;
> -        task->user_data = user_data;
> -
> -        CHANNEL_DEBUG(channel, "Insert a xfer task:%d to task list", task-
> >id);
> -        g_hash_table_insert(c->file_xfer_tasks,
> -                            GUINT_TO_POINTER(task->id),
> -                            task);
> -        g_signal_connect(task, "finished", G_CALLBACK(task_finished),
> channel);
> -        g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> task);
> -
> -        g_file_read_async(files[i],
> -                          G_PRIORITY_DEFAULT,
> -                          cancellable,
> -                          file_xfer_read_async_cb,
> -                          g_object_ref(task));
> -        task->pending = TRUE;
> -
> -        /* if we created a per-task cancellable above, free it */
> -        if (!cancellable)
> -            g_object_unref(task_cancellable);
> -    }
> +    g_hash_table_remove(channel->priv->file_xfer_tasks,
> GUINT_TO_POINTER(task_id));
>  }
>  
>  /**
> @@ -3164,6 +3125,7 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
>                                  gpointer user_data)
>  {
>      SpiceMainChannelPrivate *c = channel->priv;
> +    GList *tasks, *it;
>  
>      g_return_if_fail(channel != NULL);
>      g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> @@ -3180,16 +3142,26 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
>          return;
>      }
>  
> -    file_xfer_send_start_msg_async(channel,
> -                                   sources,
> -                                   flags,
> -                                   cancellable,
> -                                   progress_callback,
> -                                   progress_callback_data,
> -                                   file_xfer_flush_callback,
> -                                   NULL,
> -                                   callback,
> -                                   user_data);
> +    tasks = spice_file_transfer_task_create_tasks(channel,
> +                                                  sources,
> +                                                  flags,
> +                                                  cancellable,
> +                                                  progress_callback,
> +                                                  progress_callback_data,
> +                                                  file_xfer_flush_callback,
> +                                                  NULL,
> +                                                  callback,
> +                                                  user_data);
> +    for (it = tasks; it != NULL; it = it->next) {
> +        SpiceFileTransferTask *task = SPICE_FILE_TRANSFER_TASK(it->data);
> +        guint32 task_id = spice_file_transfer_task_get_id(task);
> +
> +        g_hash_table_insert(c->file_xfer_tasks, GUINT_TO_POINTER(task_id),
> task);
> +        g_signal_connect(task, "finished", G_CALLBACK(task_finished),
> channel);
> +        g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> task);
> +        spice_file_transfer_task_start_task(task);
> +    }
> +    g_list_free(tasks);
>  }
>  
>  /**
> @@ -3282,6 +3254,60 @@ static void
> spice_file_transfer_task_flush_done(SpiceFileTransferTask *self, GEr
>      file_xfer_continue_read(self);
>  }
>  
> +static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel
> *channel,
> +                                                    GFile **files,
> +                                                    GFileCopyFlags flags,
> +                                                    GCancellable
> *cancellable,
> +                                                    GFileProgressCallback
> progress_callback,
> +                                                    gpointer
> progress_callback_data,
> +                                                    SpiceFileTransferTaskFlus
> hCb flush_callback,
> +                                                    gpointer
> flush_callback_data,
> +                                                    GAsyncReadyCallback
> callback,
> +                                                    gpointer user_data)
> +{
> +    SpiceFileTransferTask *task;
> +    gint i;
> +    GList *tasks = NULL;
> +
> +    for (i = 0; files[i] != NULL && !g_cancellable_is_cancelled(cancellable);
> i++) {
> +        GCancellable *task_cancellable = cancellable;
> +        /* if a cancellable object was not provided for the overall
> operation,
> +         * create a separate object for each file so that they can be
> cancelled
> +         * separately  */
> +        if (!task_cancellable)
> +            task_cancellable = g_cancellable_new();
> +
> +        task = spice_file_transfer_task_new(channel, files[i],
> task_cancellable);
> +        task->flags = flags;
> +        task->progress_callback = progress_callback;
> +        task->progress_callback_data = progress_callback_data;
> +        task->flush_callback = flush_callback;
> +        task->flush_callback_data = flush_callback_data;
> +        task->callback = callback;
> +        task->user_data = user_data;
> +
> +        SPICE_DEBUG("Insert a xfer task:%d to task list", task->id);
> +        tasks = g_list_prepend(tasks, task);
> +
> +        /* if we created a per-task cancellable above, free it */
> +        if (!cancellable)
> +            g_object_unref(task_cancellable);
> +    }
> +
> +    return g_list_reverse(tasks);
> +}
> +
> +static void spice_file_transfer_task_start_task(SpiceFileTransferTask *self)
> +{
> +    g_return_if_fail(self != NULL);
> +    self->pending = TRUE;
> +    g_file_read_async(self->file,
> +                      G_PRIORITY_DEFAULT,
> +                      self->cancellable,
> +                      file_xfer_read_async_cb,
> +                      g_object_ref(self));
> +}
> +
>  static void
>  spice_file_transfer_task_get_property(GObject *object,
>                                        guint property_id,


Otherwise it looks OK

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]