On Thu, May 19, 2016 at 01:21:46PM +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 and also the progress_callback passed from Application.
>
> As pointed in the fix 113093dd00a1cf10f6d3c3589b7589a184cec081, the
> progress_callback should provide information of the whole transfer
> operation; For that reason, there is no need to keep progress_callback
> and progress_callback_data per SpiceFileTransferTask but per
> FileTransferOperation.
>
> The file_xfer_tasks hash table now has FileTransferOperation instead
> of SpiceFileTransferTask. To improve handling this new operation, I've
> created the helpers:
>
> * file_transfer_operation_send_progress
> * file_transfer_operation_free_op
> * file_transfer_operation_reset_all
> * file_transfer_operation_find_task_by_id
> * file_transfer_operation_remove_task
>
> This change is related to split SpiceFileTransferTask from
> channel-main.
> ---
> src/channel-main.c | 203 ++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 138 insertions(+), 65 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 28ffe80..efe559c 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -90,8 +90,6 @@ static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> GFile **files,
> GFileCopyFlags flags,
> GCancellable *cancellable,
> - GFileProgressCallback progress_callback,
> - gpointer progress_callback_data,
> SpiceFileTransferTaskFlushCb flush_callback,
> gpointer flush_callback_data,
> GAsyncReadyCallback callback,
> @@ -109,8 +107,6 @@ struct _SpiceFileTransferTaskPrivate
> GFileInputStream *file_stream;
> GFileCopyFlags flags;
> GCancellable *cancellable;
> - GFileProgressCallback progress_callback;
> - gpointer progress_callback_data;
> SpiceFileTransferTaskFlushCb flush_callback;
> gpointer flush_callback_data;
> GAsyncReadyCallback callback;
> @@ -153,6 +149,15 @@ typedef struct {
> SpiceDisplayState display_state;
> } SpiceDisplayConfig;
>
> +typedef struct {
> + GList *tasks;
> + SpiceMainChannel *channel;
> + GFileProgressCallback progress_callback;
> + gpointer progress_callback_data;
> + goffset total_sent;
> + goffset transfer_size;
> +} FileTransferOperation;
> +
> struct _SpiceMainChannelPrivate {
> enum SpiceMouseMode mouse_mode;
> enum SpiceMouseMode requested_mouse_mode;
> @@ -254,6 +259,13 @@ static void file_xfer_flushed(SpiceMainChannel *channel, gboolean success);
> 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_send_progress(SpiceMainChannel *channel, SpiceFileTransferTask *xfer_task);
> +static void file_transfer_operation_free_op(FileTransferOperation *op);
> +static void file_transfer_operation_reset_all(SpiceMainChannel *channel);
> +static SpiceFileTransferTask *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> + guint32 task_id);
> +static void file_transfer_operation_remove_task(SpiceMainChannel *channel, SpiceFileTransferTask *task);
> +
> /* ------------------------------------------------------------------ */
>
> static const char *agent_msg_types[] = {
> @@ -312,8 +324,7 @@ 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->file_xfer_tasks = g_hash_table_new(g_direct_hash, g_direct_equal);
> c->flushing = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL,
> g_object_unref);
> c->cancellable_volume_info = g_cancellable_new();
> @@ -467,9 +478,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;
> @@ -478,15 +486,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);
> + file_transfer_operation_reset_all(channel);
> file_xfer_flushed(channel, FALSE);
> }
>
> @@ -1875,7 +1875,9 @@ static void file_xfer_data_flushed_cb(GObject *source_object,
> SpiceMainChannel *channel = (SpiceMainChannel *)source_object;
> GError *error = NULL;
>
> - file_xfer_flush_finish(channel, res, &error);
> + if (file_xfer_flush_finish(channel, res, &error))
> + file_transfer_operation_send_progress(channel, self);
> +
> spice_file_transfer_task_flush_done(self, error);
> }
>
> @@ -1899,7 +1901,9 @@ static void file_xfer_flush_callback(SpiceFileTransferTask *xfer_task,
> {
> SpiceMainChannel *main_channel;
> GCancellable *cancellable;
> + FileTransferOperation *xfer_op = user_data;
>
> + xfer_op->total_sent += count;
> file_xfer_queue(xfer_task, buffer, count);
> if (count == 0)
> return;
> @@ -2141,7 +2145,7 @@ static void main_agent_handle_msg(SpiceChannel *channel,
> SpiceFileTransferTask *task;
> VDAgentFileXferStatusMessage *msg = payload;
>
> - task = g_hash_table_lookup(c->file_xfer_tasks, GUINT_TO_POINTER(msg->id));
> + task = file_transfer_operation_find_task_by_id(self, msg->id);
> if (task != NULL) {
> spice_file_transfer_task_handle_status(task, msg);
> } else {
> @@ -3035,21 +3039,22 @@ static void file_xfer_on_file_info(SpiceFileTransferTask *xfer_task,
> GFileInfo *info,
> gpointer data)
> {
> - SpiceMainChannel *channel;
> GKeyFile *keyfile;
> VDAgentFileXferStartMessage msg;
> gchar *string, *basename;
> guint64 file_size;
> gsize data_len;
> GError *error = NULL;
> + FileTransferOperation *xfer_op;
>
> g_return_if_fail(info != NULL);
> -
> - channel = SPICE_MAIN_CHANNEL(data);
> + xfer_op = data;
>
> basename = g_file_info_get_attribute_as_string(info, G_FILE_ATTRIBUTE_STANDARD_NAME);
> file_size = g_file_info_get_attribute_uint64(info, G_FILE_ATTRIBUTE_STANDARD_SIZE);
>
> + xfer_op->transfer_size += file_size;
> +
> keyfile = g_key_file_new();
> g_key_file_set_string(keyfile, "vdagent-file-xfer", "name", basename);
> g_key_file_set_uint64(keyfile, "vdagent-file-xfer", "size", file_size);
> @@ -3065,17 +3070,107 @@ static void file_xfer_on_file_info(SpiceFileTransferTask *xfer_task,
>
> /* Create file-xfer start message */
> g_object_get(xfer_task, "id", &msg.id, NULL);
> - agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_START,
> + agent_msg_queue_many(xfer_op->channel, VD_AGENT_FILE_XFER_START,
> &msg, sizeof(msg),
> string, data_len + 1, NULL);
> g_free(string);
> - spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE);
> + spice_channel_wakeup(SPICE_CHANNEL(xfer_op->channel), FALSE);
> return;
>
> failed:
> spice_file_transfer_task_completed(xfer_task, error);
> }
>
> +static void file_transfer_operation_send_progress(SpiceMainChannel *channel, SpiceFileTransferTask *xfer_task)
> +{
> + FileTransferOperation *xfer_op;
> + guint32 task_id;
> +
> + g_object_get (xfer_task, "id", &task_id, NULL);
> + xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks, GUINT_TO_POINTER(task_id));
> + g_return_if_fail(xfer_op != NULL);
> +
> + if (xfer_op->progress_callback)
> + xfer_op->progress_callback(xfer_op->total_sent,
> + xfer_op->transfer_size,
> + xfer_op->progress_callback_data);
> +}
> +
> +static void file_transfer_operation_free_op(FileTransferOperation *op)
> +{
> + g_return_if_fail(op != NULL);
> +
> + /* SpiceFileTransferTask itself is freed after it emits "finish" */
> + if (op->tasks != NULL)
> + g_list_free(op->tasks);
> +
> + g_free(op);
> +}
> +
> +static void file_transfer_operation_reset_all(SpiceMainChannel *channel)
> +{
> + GList *list_op, *it_op;
I need to be more careful here.
> +
> + list_op = g_hash_table_get_values(channel->priv->file_xfer_tasks);
each task-id is mapped to a FileTransferOperation, so the loop below
is likely passing more then once to each FileTransferOperation causing a
double free.
I'll send a v2 with improved tests :)
> + for (it_op = list_op; it_op != NULL; it_op = it_op->next) {
> + FileTransferOperation *op = it_op->data;
> + GList *it;
> + for (it = op->tasks; it != NULL; it = it->next) {
> + SpiceFileTransferTask *xfer_task = SPICE_FILE_TRANSFER_TASK(it->data);
> + GError *error = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> + "Agent connection closed");
> + spice_file_transfer_task_completed(xfer_task, error);
> + }
> + file_transfer_operation_free_op(op);
> + }
> + g_list_free(list_op);
> +}
> +
> +static SpiceFileTransferTask *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> + guint32 task_id)
> +{
> + FileTransferOperation *op;
> + GList *it;
> +
> + op = g_hash_table_lookup (channel->priv->file_xfer_tasks, GUINT_TO_POINTER(task_id));
> + g_return_val_if_fail (op != NULL, NULL);
> +
> + for (it = op->tasks; it != NULL; it = it->next) {
> + SpiceFileTransferTask *task = SPICE_FILE_TRANSFER_TASK(it->data);
> + guint32 id;
> + g_object_get(task, "id", &id, NULL);
> + if (id == task_id)
> + return task;
> + }
> + return NULL;
> +}
> +
> +static void file_transfer_operation_remove_task(SpiceMainChannel *channel, SpiceFileTransferTask *task)
> +{
> + FileTransferOperation *op;
> + guint32 task_id;
> +
> + g_object_get (task, "id", &task_id, NULL);
> + op = g_hash_table_lookup(channel->priv->file_xfer_tasks, GUINT_TO_POINTER(task_id));
> + if (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(task);
> + return;
> + }
> + /* Remove and free SpiceFileTransferTask */
> + op->tasks = g_list_remove(op->tasks, task);
> + g_object_unref(task);
> +
> + /* Keep file-xfer-tasks up to date. If no more elements, operation is over */
> + g_hash_table_remove(channel->priv->file_xfer_tasks, GUINT_TO_POINTER(task_id));
> +
> + /* No more pending operations */
> + if (op->tasks == NULL)
> + file_transfer_operation_free_op(op);
> +}
> +
> static void task_finished(SpiceFileTransferTask *task,
> GError *error,
> gpointer data)
> @@ -3093,7 +3188,7 @@ static void task_finished(SpiceFileTransferTask *task,
> }
>
> g_object_get(task, "id", &task_id, NULL);
> - g_hash_table_remove(channel->priv->file_xfer_tasks, GUINT_TO_POINTER(task_id));
> + file_transfer_operation_remove_task(channel, task);
> }
>
> /**
> @@ -3139,7 +3234,8 @@ void spice_main_file_copy_async(SpiceMainChannel *channel,
> gpointer user_data)
> {
> SpiceMainChannelPrivate *c = channel->priv;
> - GList *tasks, *it;
> + GList *it;
> + FileTransferOperation *xfer_op;
>
> g_return_if_fail(channel != NULL);
> g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> @@ -3156,28 +3252,29 @@ void spice_main_file_copy_async(SpiceMainChannel *channel,
> return;
> }
>
> - 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) {
> + xfer_op = g_new0(FileTransferOperation, 1);
> + xfer_op->progress_callback = progress_callback;
> + xfer_op->progress_callback_data = progress_callback_data;
> + xfer_op->channel = channel;
> + xfer_op->tasks = spice_file_transfer_task_create_tasks(channel,
> + sources,
> + flags,
> + cancellable,
> + file_xfer_flush_callback,
> + xfer_op,
> + callback,
> + user_data);
> + for (it = xfer_op->tasks; it != NULL; it = it->next) {
> SpiceFileTransferTask *task = SPICE_FILE_TRANSFER_TASK(it->data);
> guint32 task_id;
>
> g_object_get(task, "id", &task_id, NULL);
> - g_hash_table_insert(c->file_xfer_tasks, GUINT_TO_POINTER(task_id), task);
> - g_signal_connect(task, "file-info", G_CALLBACK(file_xfer_on_file_info), channel);
> + g_hash_table_insert(c->file_xfer_tasks, GUINT_TO_POINTER(task_id), xfer_op);
> + g_signal_connect(task, "file-info", G_CALLBACK(file_xfer_on_file_info), xfer_op);
> 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);
> }
>
> /**
> @@ -3240,26 +3337,6 @@ static void spice_file_transfer_task_flush_done(SpiceFileTransferTask *self, GEr
> }
> }
>
> - if (self->priv->progress_callback) {
> - goffset read = 0;
> - goffset total = 0;
> - SpiceMainChannel *main_channel = self->priv->channel;
> - GHashTableIter iter;
> - gpointer key, value;
> -
> - /* 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, main_channel->priv->file_xfer_tasks);
> - while (g_hash_table_iter_next(&iter, &key, &value)) {
> - SpiceFileTransferTask *t = (SpiceFileTransferTask *)value;
> - read += t->priv->read_bytes;
> - total += t->priv->file_size;
> - }
> -
> - self->priv->progress_callback(read, total, self->priv->progress_callback_data);
> - }
> -
> /* Read more data */
> file_xfer_continue_read(self);
> }
> @@ -3268,8 +3345,6 @@ static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> GFile **files,
> GFileCopyFlags flags,
> GCancellable *cancellable,
> - GFileProgressCallback progress_callback,
> - gpointer progress_callback_data,
> SpiceFileTransferTaskFlushCb flush_callback,
> gpointer flush_callback_data,
> GAsyncReadyCallback callback,
> @@ -3289,8 +3364,6 @@ static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
>
> task = spice_file_transfer_task_new(channel, files[i], task_cancellable);
> task->priv->flags = flags;
> - task->priv->progress_callback = progress_callback;
> - task->priv->progress_callback_data = progress_callback_data;
> task->priv->flush_callback = flush_callback;
> task->priv->flush_callback_data = flush_callback_data;
> task->priv->callback = callback;
> --
> 2.5.5
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel