On Thu, 2016-06-23 at 19:37 +0200, Victor Toso wrote:
> Before this patch, the progress_callback is being called from
> SpiceFileTransferTask each time that some data is flushed to the agent
> of this given xfer-task. The progress value is computed by iterating
> on all available xfer-tasks.
>
> This patch intend to fix/change:
>
> * The progress_callback should be called only with information related
> to the FileTransferOperation of given xfer-task; This was also
> suggested by 113093dd00a1cf10f6d3c3589b7589a184cec081;
>
> * The progress_callback should not be done from SpiceFileTransferTask
> context by (proposed) design. As the transfer operation starts in
> spice_main_file_copy_async(), FileTransferOperation should handle
> these calls.
> ---
> src/channel-main.c | 102 ++++++++++++++++++++++++++++++--------------------
> ---
> 1 file changed, 57 insertions(+), 45 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 117c735..fba5b7f 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -61,8 +61,6 @@ static GHashTable
> *spice_file_transfer_task_create_tasks(GFile **files,
> SpiceMainChannel
> *channel,
> GFileCopyFlags
> flags,
> GCancellable
> *cancellable,
> - GFileProgressCallbac
> k progress_callback,
> - gpointer
> progress_callback_data,
> GAsyncReadyCallback
> callback,
> gpointer user_data);
> static void spice_file_transfer_task_init_task_async(SpiceFileTransferTask
> *self,
> @@ -78,6 +76,8 @@ static gssize
> spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
> GAsyncResult *result,
> char **buffer,
> GError **error);
> +static guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> *self);
> +static guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask
> *self);
>
> /**
> * SECTION:file-transfer-task
> @@ -108,8 +108,6 @@ struct _SpiceFileTransferTask
> GFileInputStream *file_stream;
> GFileCopyFlags flags;
> GCancellable *cancellable;
> - GFileProgressCallback progress_callback;
> - gpointer progress_callback_data;
> GAsyncReadyCallback callback;
> gpointer user_data;
> char *buffer;
> @@ -162,6 +160,12 @@ typedef struct {
> typedef struct {
> GHashTable *xfer_task_ht;
> SpiceMainChannel *channel;
> + GFileProgressCallback progress_callback;
> + gpointer progress_callback_data;
> + struct {
> + goffset total_sent;
> + goffset transfer_size;
> + } stats;
> } FileTransferOperation;
>
> struct _SpiceMainChannelPrivate {
> @@ -274,6 +278,7 @@ static SpiceFileTransferTask
> *file_transfer_operation_find_task_by_id(SpiceMainC
> static void file_transfer_operation_task_finished(SpiceFileTransferTask
> *xfer_task,
> GError *error,
> gpointer userdata);
> +static void file_transfer_operation_send_progress(SpiceFileTransferTask
> *xfer_task);
>
> /* ------------------------------------------------------------------ */
>
> @@ -1884,39 +1889,6 @@ static void file_xfer_close_cb(GObject *object,
> g_object_unref(self);
> }
>
> -static void file_xfer_send_progress(SpiceFileTransferTask *xfer_task)
> -{
> - goffset read = 0;
> - goffset total = 0;
> - GHashTableIter iter;
> - gpointer key, value;
> - SpiceMainChannel *channel;
> -
> - g_return_if_fail(xfer_task != NULL);
> -
> - if (!xfer_task->progress_callback)
> - return;
> -
> - 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 */
> - /* 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;
> -
> - t = file_transfer_operation_find_task_by_id(channel,
> GPOINTER_TO_UINT(key));
> - read += t->read_bytes;
> - total += t->file_size;
> - }
> -
> - xfer_task->progress_callback(read, total, xfer_task-
> >progress_callback_data);
> -}
> -
> static void file_xfer_data_flushed_cb(GObject *source_object,
> GAsyncResult *res,
> gpointer user_data)
> @@ -1931,7 +1903,7 @@ static void file_xfer_data_flushed_cb(GObject
> *source_object,
> return;
> }
>
> - file_xfer_send_progress(self);
> + file_transfer_operation_send_progress(self);
>
> if (spice_util_get_debug()) {
> const GTimeSpan interval = 20 * G_TIME_SPAN_SECOND;
> @@ -1971,11 +1943,13 @@ static void file_xfer_read_async_cb(GObject
> *source_object,
> GAsyncResult *res,
> gpointer user_data)
> {
> + FileTransferOperation *xfer_op;
> SpiceFileTransferTask *xfer_task;
> SpiceMainChannel *channel;
> gssize count;
> char *buffer;
> GCancellable *cancellable;
> + guint32 task_id;
> GError *error = NULL;
>
> xfer_task = SPICE_FILE_TRANSFER_TASK(source_object);
> @@ -1993,6 +1967,10 @@ static void file_xfer_read_async_cb(GObject
> *source_object,
> /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent */
> return;
>
> + task_id = spice_file_transfer_task_get_id(xfer_task);
> + xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks,
> GUINT_TO_POINTER(task_id));
> + xfer_op->stats.total_sent += count;
> +
> cancellable = spice_file_transfer_task_get_cancellable(xfer_task);
> file_xfer_flush_async(channel, cancellable, file_xfer_data_flushed_cb,
> xfer_task);
> }
> @@ -3019,6 +2997,7 @@ static void file_xfer_init_task_async_cb(GObject *obj,
> GAsyncResult *res, gpoint
> VDAgentFileXferStartMessage msg;
> guint64 file_size;
> gsize data_len;
> + FileTransferOperation *xfer_op;
> GError *error = NULL;
>
> xfer_task = SPICE_FILE_TRANSFER_TASK(obj);
> @@ -3031,6 +3010,9 @@ static void file_xfer_init_task_async_cb(GObject *obj,
> GAsyncResult *res, gpoint
> 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 = data;
> + xfer_op->stats.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);
> @@ -3129,6 +3111,11 @@ static void
> file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
> return;
> }
>
> + if (error) {
> + xfer_op->stats.total_sent -=
> spice_file_transfer_task_get_bytes_read(xfer_task);
> + xfer_op->stats.transfer_size -=
> spice_file_transfer_task_get_file_size(xfer_task);
> + }
> +
So, this can cause the "bytes read" value to decrease from one update to the
next. I don't think that will cause any problems with the current client code,
but it could be a little bit surprising to the API user. One alternative may be
to simply consider the task to be completely read when it hits an error. In
other words, something like:
if (error) {
xfer_op->stats.transfer_size -= (file_size - bytes_read)
}
That way, the total size left to transfer drops slightly, but the number of
bytes read remains monotonically increasing. I don't know if this is necessarily
better than your approach, but I just thought I'd throw it out for discussion.
opinion?
> /* Remove and free SpiceFileTransferTask */
> g_hash_table_remove(xfer_op->xfer_task_ht, GUINT_TO_POINTER(task_id));
> g_object_unref(xfer_task);
> @@ -3141,6 +3128,23 @@ static void
> file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
> file_transfer_operation_end(xfer_op);
> }
>
> +static void file_transfer_operation_send_progress(SpiceFileTransferTask
> *xfer_task)
> +{
> + FileTransferOperation *xfer_op;
> + SpiceMainChannel *channel;
> + guint32 task_id;
> +
> + channel = spice_file_transfer_task_get_channel(xfer_task);
> + task_id = spice_file_transfer_task_get_id(xfer_task);
> + 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->stats.total_sent,
> + xfer_op->stats.transfer_size,
> + xfer_op->progress_callback_data);
> +}
> +
This strikes me as a little bit convoluted. The function is ostensibly a
FileTransferOperation method (by the name and by the behavior), yet the function
argument is SpiceFileTransferTask*. And to get from SpiceFileTransferTask to
FileTransferOperation, we have to first get the SpiceMainChannel and ID from the
task, then look up the operation from a hash table in the channel. This is one
of the reasons that I thought it might be better for the task structure to have
a reference to its operation rather than storing the operation in a hash table
in the channel indexed on task ID. But perhaps my solution won't work out very
well in real life, and I may just being overly nitpick-y here. Feel free to tell
me so if I am ;)
> static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel
> *channel,
> GFile *file,
> GCancellable
> *cancellable);
> @@ -3210,12 +3214,12 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
>
> xfer_op = g_new0(FileTransferOperation, 1);
> xfer_op->channel = channel;
> + xfer_op->progress_callback = progress_callback;
> + xfer_op->progress_callback_data = progress_callback_data;
> xfer_op->xfer_task_ht = spice_file_transfer_task_create_tasks(sources,
> channel,
> flags,
> cancellable
> ,
> - progress_ca
> llback,
> - progress_ca
> llback_data,
> callback,
> user_data);
> g_hash_table_iter_init(&iter, xfer_op->xfer_task_ht);
> @@ -3234,7 +3238,7 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
>
> spice_file_transfer_task_init_task_async(xfer_task,
> file_xfer_init_task_async_cb
> ,
> - NULL);
> + xfer_op);
> }
> }
>
> @@ -3279,6 +3283,18 @@ static GCancellable
> *spice_file_transfer_task_get_cancellable(SpiceFileTransferT
> return self->cancellable;
> }
>
> +static guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> *self)
> +{
> + g_return_val_if_fail(self != NULL, 0);
> + return self->file_size;
> +}
> +
> +static guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask
> *self)
> +{
> + g_return_val_if_fail(self != NULL, 0);
> + return self->read_bytes;
> +}
> +
> /* Helper function which only creates a SpiceFileTransferTask per GFile
> * in @files and returns a HashTable mapping task-id to the task itself
> * Note that the HashTable does not free its values uppon destruction:
> @@ -3288,8 +3304,6 @@ static GHashTable
> *spice_file_transfer_task_create_tasks(GFile **files,
> SpiceMainChannel
> *channel,
> GFileCopyFlags
> flags,
> GCancellable
> *cancellable,
> - GFileProgressCallbac
> k progress_callback,
> - gpointer
> progress_callback_data,
> GAsyncReadyCallback
> callback,
> gpointer user_data)
> {
> @@ -3312,8 +3326,6 @@ static GHashTable
> *spice_file_transfer_task_create_tasks(GFile **files,
>
> xfer_task = spice_file_transfer_task_new(channel, files[i],
> task_cancellable);
> xfer_task->flags = flags;
> - xfer_task->progress_callback = progress_callback;
> - xfer_task->progress_callback_data = progress_callback_data;
> xfer_task->callback = callback;
> xfer_task->user_data = user_data;
>
looks good other than discussion points above
Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel