On Tue, 2016-06-28 at 14:39 +0200, Victor Toso wrote:
> 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,
> > > - GFileProgressCal
> > > lbac
> > > k progress_callback,
> > > - gpointer
> > > progress_callback_data,
> > > GAsyncReadyCallb
> > > ack
> > > 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 % |
should be: 800kb ? 600kb ? | 75.00 % ?
> | Cancel C | 1600kb | 500kb | 31.50 % |
should be: 0kb ? | 0 kb? | 0.00 % or 100.00 %
Or did I understand wrong?
>
> 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 % |
should be: 1300kb | 1100kb | 84.62 % |
> | Cancel C | 2200kb | 1100kb | 50.00 % |
should be: 1100kb | 1100kb | 100.00 %
Essentially, subtract the "Pending" value from transfer_size when a task is
cancelled
>
> 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 % |
should be: 2200kb | 91.67 %
> | Cancel C | 2400kb | 1300kb | 54.16 % |
should be: 2400kb | 100.00 %
>
> As expected, it also always increase the percentage.
Your numbers didn't really match this statement ;)
>
> 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 :)
well, I think approach 3 is a bit of a lie since it implies that we sent a lot
more data than we really did. If a client wanted to use the amount of data sent
to try to calculate and display a transfer rate in the UI, that would not work.
Similarly, you couldn't really use option 1 for this purpose since it implies
that we sent less data than we actually did. So I would personally still go for
option 2.
>
> >
> >
> >
> >
> > >
> > > /* 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 :)
Yes, we can address it later.
>
> >
> >
> > >
> > > 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,
> > > cancell
> > > able
> > > ,
> > > - progres
> > > s_ca
> > > llback,
> > > - progres
> > > s_ca
> > > llback_data,
> > > callbac
> > > k,
> > > user_da
> > > ta);
> > > 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_asyn
> > > c_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,
> > > - GFileProgressCal
> > > lbac
> > > k progress_callback,
> > > - gpointer
> > > progress_callback_data,
> > > GAsyncReadyCallb
> > > ack
> > > 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