[Spice-devel] [spice-gtk v3 03/16] file-xfer: introduce create_tasks and start_task
Victor Toso
victortoso at redhat.com
Sun Jun 5 07:40:13 UTC 2016
Hi,
On Thu, Jun 02, 2016 at 11:44:42AM -0500, Jonathon Jongsma wrote:
> 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.
>
I agree! I'll change it
> > +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 at redhat.com>
More information about the Spice-devel
mailing list