Re: [spice-gtk v4 14/24] file-xfer: call progress_callback per FileTransferOperation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]