Hi,
On Wed, Jul 06, 2016 at 11:08:53AM -0500, Jonathon Jongsma wrote:
> On Tue, 2016-07-05 at 15:07 +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;
> >
> > * In case a SpiceFileTransferTask is cancelled or has an error, we
> > remove the remaining bytes (not sent) of this file from the
> > transfer_size;
> >
> > * 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 | 103 ++++++++++++++++++++++++++++++--------------------
> > ---
> > 1 file changed, 58 insertions(+), 45 deletions(-)
> >
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index e57d2ba..c5540e7 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;
> > @@ -161,6 +159,12 @@ typedef struct {
> > typedef struct {
> > GHashTable *xfer_task;
> > SpiceMainChannel *channel;
> > + GFileProgressCallback progress_callback;
> > + gpointer progress_callback_data;
> > + struct {
> > + goffset total_sent;
> > + goffset transfer_size;
> > + } stats;
> > } FileTransferOperation;
> >
> > struct _SpiceMainChannelPrivate {
> > @@ -273,6 +277,7 @@ static SpiceFileTransferTask
> > *spice_main_channel_find_xfer_task_by_task_id(Spice
> > static void file_transfer_operation_task_finished(SpiceFileTransferTask
> > *xfer_task,
> > GError *error,
> > gpointer userdata);
> > +static void file_transfer_operation_send_progress(SpiceFileTransferTask
> > *xfer_task);
> >
> > /* ------------------------------------------------------------------ */
> >
> > @@ -1883,39 +1888,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 = spice_main_channel_find_xfer_task_by_task_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)
> > @@ -1930,7 +1902,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);
> > @@ -1994,6 +1968,10 @@ static void file_xfer_read_async_cb(GObject
> > *source_object,
> > 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);
> > }
> > @@ -3027,6 +3005,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);
> > @@ -3039,6 +3018,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);
> > @@ -3144,6 +3126,12 @@ static void
> > file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
> > return;
> > }
> >
> > + if (error) {
> > + guint64 file_size =
> > spice_file_transfer_task_get_file_size(xfer_task);
> > + guint64 bytes_read =
> > spice_file_transfer_task_get_bytes_read(xfer_task);
> > + xfer_op->stats.transfer_size -= (file_size - bytes_read);
> > + }
> > +
>
> It might be useful to have a code comment here explaining why we're
> doing this? or maybe it's obvious?
Considering that we discussed a bit about it, I agree that we should
have some explanation in the code as well. (I put an extra bullet about
it in the commit log)
<snip>
/* On error or cancellation of a SpiceFileTransferTask we remove the
* remaining bytes from transfer-size in order to keep the coherence of
* the information we provide to the user (total-sent and transfer-size)
* in the progress-callback */
>
> > /* Remove and free SpiceFileTransferTask */
> > g_hash_table_remove(xfer_op->xfer_task, GUINT_TO_POINTER(task_id));
> > g_object_unref(xfer_task);
> > @@ -3156,6 +3144,23 @@ static void
> > file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
> > file_transfer_operation_free(xfer_op);
> > }
> >
> > +static void file_transfer_operation_send_progress(SpiceFileTransferTask
> > *xfer_task)
jjjjjjjjjjjjjjjjj> > +{
> > + 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);
> > +}
> > +
> > static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel
> > *channel,
> > GFile *file,
> > GCancellable
> > *cancellable);
> > @@ -3225,12 +3230,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 = 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);
> > @@ -3248,7 +3253,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);
> > }
> > }
> >
> > @@ -3293,6 +3298,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 upon destruction:
> > @@ -3302,8 +3319,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)
> > {
> > @@ -3327,8 +3342,6 @@ static GHashTable
> > *spice_file_transfer_task_create_tasks(GFile **files,
> > /* FIXME: Move the xfer-task initialization to
> > spice_file_transfer_task_new() */
> > 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;
> >
>
> Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
Thanks,
> _______________________________________________
> 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