Hi,
On Mon, Jun 27, 2016 at 11:27:38AM -0500, Jonathon Jongsma wrote:
> 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)
> }
or:
if (error) {
xfer_op->stats.total_sent += (file_size - bytes_read)
}
The idea is the same (set task as complete on error) but we can have
different values in percentage.
> 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?
I'm thinking about it more carefully now and I think I got what you
mean. It really depends on how read_bytes and transfer_size will be used
by the API user so, we can define the behavior that we want.
To make both proposals clear, let's take an example of a transfer
operation of three files where one of them will be cancelled.
| File | Size | Sent | Pending |
| A | 800kb | 100kb | 700kb |
| B | 800kb | 400kb | 400kb |
| C | 800kb | 600kb | 200kb |
----------------------------------
Total: 2400kb | 1100kb | 1300kb |
1-) With proposal from this patch
| -------- | transfer_size | total_sent | percentage |
| Before | 2400kb | 1100kb | 45.83 % |
| Cancel A | 1600kb | 1000kb | 62.50 % |
| Cancel B | 1600kb | 700kb | 43.75 % |
| Cancel C | 1600kb | 500kb | 31.50 % |
What I like is that the percentage is 100% related to the actual read
and actual transfer size; What I don't like is that the percentage might
go up or down based on cancel/errors.
2-) With your proposal (task complete by changing transfer-size)
| -------- | transfer_size | total_sent | percentage |
| Before | 2400kb | 1100kb | 45.83 % |
| Cancel A | 1700kb | 1100kb | 64.70 % |
| Cancel B | 2000kb | 1100kb | 55.75 % |
| Cancel C | 2200kb | 1100kb | 50.00 % |
What I like is that the percentage always increase which would really
match the expected behavior of cancel/error of a task
3-) With your proposal but changing the total_sent instead
| -------- | transfer_size | total_sent | percentage |
| Before | 2400kb | 1100kb | 45.83 % |
| Cancel A | 2400kb | 1800kb | 75.00 % |
| Cancel B | 2400kb | 1500kb | 62.50 % |
| Cancel C | 2400kb | 1300kb | 54.16 % |
As expected, it also always increase the percentage.
If there is not real standard behind this and it is up to us to decide,
I would either stick with option (1) and in case of cancel/error we are
sending the correct value of pending files and the user API could handle
the progress data as it wishes; or I would go for option (3) as it is
simpler to say that "we add the pending bytes to total_sent on
cancel/error" but also, the percentage is what you should expect in case
that file was transferred completely (75% if File A was transferred
while B and C were in the 'before' state)
Let me know what you think :)
>
>
>
> > /* 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 ;)
Not at all, I agree with you. Still, it is a bit hard to make everything
play nice in one batch! In my opinion, the file_xfer_* functions were
not meant to work per file-transfer-operation but per-file-transfer and
so we get this problem above.
In this case, this function is called when data is flushed at
file_xfer_data_flushed_cb and I really think we should have a xfer_op
pointer.
I think we can better organize this if we use SpiceFileTransferTask as
source_object and pass FileTransferOperations as user_data of this
read_async/flush functions.
I would prefer to address this later if you don't feel it is a blocker,
too many patches already :)
>
> > 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>
As always, many thanks!
toso
>
>
> _______________________________________________
> 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