On Tue, 2016-07-05 at 15:07 +0200, Victor Toso wrote: > Each call to spice_main_file_copy_async will now create a > FileTransferOperation which groups all SpiceFileTransferTasks of the > copy operation in a GHashTable. > > To ease the review process, this first patch introduces the structure > and organize the logic around the four following helpers: > > * spice_main_channel_reset_all_xfer_operations > * spice_main_channel_find_xfer_task_by_task_id > * file_transfer_operation_free > * file_transfer_operation_task_finished > > The _task_finished function is the new callback for "finished" signal > from SpiceFileTransferTask. > > The file_xfer_tasks GHashTable is now mapped as: > (key) SpiceFileTransferTask ID -> (value) FileTransferOperation > > This patch leaves a FIXME regarding progress_callback which will be > addressed in a later patch in this series. > > This change is related to split SpiceFileTransferTask from > channel-main. > --- > src/channel-main.c | 168 ++++++++++++++++++++++++++++++++++++++++---------- > --- > 1 file changed, 127 insertions(+), 41 deletions(-) > > diff --git a/src/channel-main.c b/src/channel-main.c > index 529c36c..8e8f6bd 100644 > --- a/src/channel-main.c > +++ b/src/channel-main.c > @@ -158,6 +158,11 @@ typedef struct { > SpiceDisplayState display_state; > } SpiceDisplayConfig; > > +typedef struct { > + GHashTable *xfer_task; > + SpiceMainChannel *channel; > +} FileTransferOperation; > + > struct _SpiceMainChannelPrivate { > enum SpiceMouseMode mouse_mode; > enum SpiceMouseMode requested_mouse_mode; > @@ -261,6 +266,14 @@ static void file_xfer_read_async_cb(GObject > *source_object, > static void spice_main_set_max_clipboard(SpiceMainChannel *self, gint max); > static void set_agent_connected(SpiceMainChannel *channel, gboolean > connected); > > +static void file_transfer_operation_free(FileTransferOperation *xfer_op); > +static void spice_main_channel_reset_all_xfer_operations(SpiceMainChannel > *channel); > +static SpiceFileTransferTask > *spice_main_channel_find_xfer_task_by_task_id(SpiceMainChannel *channel, > + gu > int32 task_id); > +static void file_transfer_operation_task_finished(SpiceFileTransferTask > *xfer_task, > + GError *error, > + gpointer userdata); > + > /* ------------------------------------------------------------------ */ > > static const char *agent_msg_types[] = { > @@ -319,10 +332,8 @@ static void spice_main_channel_init(SpiceMainChannel > *channel) > > c = channel->priv = SPICE_MAIN_CHANNEL_GET_PRIVATE(channel); > c->agent_msg_queue = g_queue_new(); > - c->file_xfer_tasks = g_hash_table_new_full(g_direct_hash, g_direct_equal, > - NULL, g_object_unref); > - c->flushing = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, > - g_object_unref); > + c->file_xfer_tasks = g_hash_table_new(g_direct_hash, g_direct_equal); > + c->flushing = g_hash_table_new(g_direct_hash, g_direct_equal); > c->cancellable_volume_info = g_cancellable_new(); > > spice_main_channel_reset_capabilties(SPICE_CHANNEL(channel)); > @@ -474,9 +485,6 @@ static void spice_channel_iterate_write(SpiceChannel > *channel) > static void spice_main_channel_reset_agent(SpiceMainChannel *channel) > { > SpiceMainChannelPrivate *c = channel->priv; > - GError *error; > - GList *tasks; > - GList *l; > > c->agent_connected = FALSE; > c->agent_caps_received = FALSE; > @@ -485,15 +493,7 @@ static void > spice_main_channel_reset_agent(SpiceMainChannel *channel) > g_clear_pointer(&c->agent_msg_data, g_free); > c->agent_msg_size = 0; > > - tasks = g_hash_table_get_values(c->file_xfer_tasks); > - for (l = tasks; l != NULL; l = l->next) { > - SpiceFileTransferTask *task = (SpiceFileTransferTask *)l->data; > - > - error = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, > - "Agent connection closed"); > - spice_file_transfer_task_completed(task, error); > - } > - g_list_free(tasks); > + spice_main_channel_reset_all_xfer_operations(channel); > file_xfer_flushed(channel, FALSE); > } > > @@ -1898,12 +1898,17 @@ static void > file_xfer_send_progress(SpiceFileTransferTask *xfer_task) > > channel = spice_file_transfer_task_get_channel(xfer_task); > > + /* FIXME: This iterates to all SpiceFileTransferTasks, including ones > from > + * different FileTransferOperation. The progress_callback should only > return > + * the info related to its FileTransferOperation */ A couple recommendations here: - "iterates to" should be "iterates over" - "ones from different FileTransferOperation" should be "ones from *a* different..." or "ones from different FileTransferOperation*s*" I assume that you closed the comment here since it's a FIXME that will be removed soon. It's a little weird to have two comments right in a row instead of one larger comment, but I guess that's fine. > /* since the progress_callback does not have a parameter to indicate > * which file the progress is associated with, report progress on all > * current transfers */ > g_hash_table_iter_init(&iter, channel->priv->file_xfer_tasks); > while (g_hash_table_iter_next(&iter, &key, &value)) { > - SpiceFileTransferTask *t = (SpiceFileTransferTask *)value; > + SpiceFileTransferTask *t; > + > + t = spice_main_channel_find_xfer_task_by_task_id(channel, > GPOINTER_TO_UINT(key)); > read += t->read_bytes; > total += t->file_size; > } > @@ -1998,11 +2003,9 @@ static void > main_agent_handle_xfer_status(SpiceMainChannel *channel, > VDAgentFileXferStatusMessage *msg) > { > SpiceFileTransferTask *xfer_task; > - SpiceMainChannelPrivate *c; > GError *error = NULL; > > - c = channel->priv; > - xfer_task = g_hash_table_lookup(c->file_xfer_tasks, GUINT_TO_POINTER(msg- > >id)); > + xfer_task = spice_main_channel_find_xfer_task_by_task_id(channel, msg- > >id); > g_return_if_fail(xfer_task != NULL); > > SPICE_DEBUG("xfer-task %u received response %u", msg->id, msg->result); > @@ -3073,18 +3076,100 @@ failed: > spice_file_transfer_task_completed(xfer_task, error); > } > > -static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel > *channel, > - GFile *file, > - GCancellable > *cancellable); > +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); > + > + /* SpiceFileTransferTask itself is freed after it emits "finish" */ > + g_hash_table_unref(xfer_op->xfer_task); > + g_free(xfer_op); > +} > + > +static void spice_main_channel_reset_all_xfer_operations(SpiceMainChannel > *channel) > +{ > + GHashTableIter iter_all_xfer_tasks; > + gpointer key, value; > + > + /* Mark each of SpiceFileTransferTask as completed due error */ > + g_hash_table_iter_init(&iter_all_xfer_tasks, channel->priv- > >file_xfer_tasks); > + while (g_hash_table_iter_next(&iter_all_xfer_tasks, &key, &value)) { > + FileTransferOperation *xfer_op = value; > + SpiceFileTransferTask *xfer_task = g_hash_table_lookup(xfer_op- > >xfer_task, key); > + GError *error; > + > + if (xfer_task == NULL) { > + spice_warning("(reset-all) can't complete task %u - completed > already?", > + GPOINTER_TO_UINT(key)); > + continue; > + } > + > + error = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED, > + "Agent connection closed"); > + spice_file_transfer_task_completed(xfer_task, error); > + } > +} > > -static void task_finished(SpiceFileTransferTask *task, > - GError *error, > - gpointer data) > +static SpiceFileTransferTask > *spice_main_channel_find_xfer_task_by_task_id(SpiceMainChannel *channel, > + gu > int32 task_id) > { > - SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data); > - g_hash_table_remove(channel->priv->file_xfer_tasks, > GUINT_TO_POINTER(task->id)); > + FileTransferOperation *xfer_op; > + > + xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks, > GUINT_TO_POINTER(task_id)); > + g_return_val_if_fail(xfer_op != NULL, NULL); > + return g_hash_table_lookup(xfer_op->xfer_task, > GUINT_TO_POINTER(task_id)); > } > > +static void file_transfer_operation_task_finished(SpiceFileTransferTask > *xfer_task, > + GError *error, > + gpointer userdata) > +{ > + SpiceMainChannel *channel; > + FileTransferOperation *xfer_op; > + guint32 task_id; > + > + channel = spice_file_transfer_task_get_channel(xfer_task); > + g_return_if_fail(channel != NULL); > + task_id = spice_file_transfer_task_get_id(xfer_task); > + g_return_if_fail(task_id != 0); > + > + if (error) { > + VDAgentFileXferStatusMessage msg; > + msg.id = task_id; > + if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { > + msg.result = VD_AGENT_FILE_XFER_STATUS_CANCELLED; > + } else { > + msg.result = VD_AGENT_FILE_XFER_STATUS_ERROR; > + } > + agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_STATUS, > + &msg, sizeof(msg), NULL); > + } > + > + xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks, > GUINT_TO_POINTER(task_id)); > + if (xfer_op == NULL) { > + /* Likely the operation has ended before the remove-task was called. > One > + * situation that this can easily happen is if the agent is > disconnected > + * while there are pending files. */ > + g_object_unref(xfer_task); > + return; > + } > + > + /* Remove and free SpiceFileTransferTask */ > + g_hash_table_remove(xfer_op->xfer_task, GUINT_TO_POINTER(task_id)); > + g_object_unref(xfer_task); > + > + /* Keep file-xfer-tasks up to date. If no more elements, operation is > over */ file-xfer-tasks -> file_xfer_tasks > + g_hash_table_remove(channel->priv->file_xfer_tasks, > GUINT_TO_POINTER(task_id)); > + > + /* No more pending operations */ > + if (g_hash_table_size(xfer_op->xfer_task) == 0) > + file_transfer_operation_free(xfer_op); > +} > + > +static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel > *channel, > + GFile *file, > + GCancellable > *cancellable); > + > /** > * spice_main_file_copy_async: > * @channel: a #SpiceMainChannel > @@ -3128,9 +3213,9 @@ void spice_main_file_copy_async(SpiceMainChannel > *channel, > gpointer user_data) > { > SpiceMainChannelPrivate *c; > - GHashTable *xfer_ht; > GHashTableIter iter; > gpointer key, value; > + FileTransferOperation *xfer_op; > > g_return_if_fail(channel != NULL); > g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel)); > @@ -3148,15 +3233,17 @@ void spice_main_file_copy_async(SpiceMainChannel > *channel, > return; > } > > - xfer_ht = spice_file_transfer_task_create_tasks(sources, > - channel, > - flags, > - cancellable, > - progress_callback, > - progress_callback_data, > - callback, > - user_data); > - g_hash_table_iter_init(&iter, xfer_ht); > + xfer_op = g_new0(FileTransferOperation, 1); > + xfer_op->channel = channel; > + xfer_op->xfer_task = spice_file_transfer_task_create_tasks(sources, > + channel, > + flags, > + cancellable, > + progress_callb > ack, > + progress_callb > ack_data, > + callback, > + user_data); > + g_hash_table_iter_init(&iter, xfer_op->xfer_task); > while (g_hash_table_iter_next(&iter, &key, &value)) { > guint32 task_id; > SpiceFileTransferTask *xfer_task = value; > @@ -3165,15 +3252,14 @@ void spice_main_file_copy_async(SpiceMainChannel > *channel, > > SPICE_DEBUG("Insert a xfer task:%u to task list", task_id); > > - g_hash_table_insert(c->file_xfer_tasks, key, > g_object_ref(xfer_task)); > - g_signal_connect(xfer_task, "finished", G_CALLBACK(task_finished), > channel); > + g_hash_table_insert(c->file_xfer_tasks, key, xfer_op); > + g_signal_connect(xfer_task, "finished", > G_CALLBACK(file_transfer_operation_task_finished), NULL); > g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0, > xfer_task); > > spice_file_transfer_task_init_task_async(xfer_task, > file_xfer_init_task_async_cb > , > NULL); > } > - g_hash_table_unref(xfer_ht); > } > > /** aside from the comment issues above, Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel