Re: [spice-gtk v3 06/16] file-xfer: a FileTransferOperation per transfer call

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

 



Hi,

On Tue, Jun 07, 2016 at 11:28:45AM -0500, Jonathon Jongsma wrote:
> On Sun, 2016-06-05 at 11:31 +0200, Victor Toso wrote:
> > On Fri, Jun 03, 2016 at 11:42:13AM -0500, Jonathon Jongsma wrote:
> > > 
> > > On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> > > > 
> > > > Each call to spice_main_file_copy_async will now create a
> > > > FileTransferOperation which groups all SpiceFileTransferTasks of the
> > > > copy operation and also the progress_callback passed from Application.
> > > > 
> > > > As pointed in the fix 113093dd00a1cf10f6d3c3589b7589a184cec081, the
> > > > progress_callback should provide information of the whole transfer
> > > > operation; For that reason, there is no need to keep progress_callback
> > > > and progress_callback_data per SpiceFileTransferTask but per
> > > > FileTransferOperation.
> > > > 
> > > > The file_xfer_tasks hash table now has FileTransferOperation instead
> > > > of SpiceFileTransferTask. To improve handling this new operation, I've
> > > > created the helpers:
> > > > 
> > > > * file_transfer_operation_send_progress
> > > > * file_transfer_operation_end
> > > > * file_transfer_operation_reset_all
> > > > * file_transfer_operation_find_task_by_id
> > > > * file_transfer_operation_task_finished
> > > > 
> > > > This change is related to split SpiceFileTransferTask from
> > > > channel-main.
> > > > ---
> > > >  src/channel-main.c | 206 ++++++++++++++++++++++++++++++++++++----------
> > > > ----
> > > > ---
> > > >  1 file changed, 139 insertions(+), 67 deletions(-)
> > > > 
> > > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > > index 72dcf1f..f36326d 100644
> > > > --- a/src/channel-main.c
> > > > +++ b/src/channel-main.c
> > > > @@ -66,8 +66,6 @@ static GList
> > > > *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> > > >                                                      GFile **files,
> > > >                                                      GFileCopyFlags flags,
> > > >                                                      GCancellable
> > > > *cancellable,
> > > > -                                                    GFileProgressCallback
> > > > progress_callback,
> > > > -                                                    gpointer
> > > > progress_callback_data,
> > > >                                                      SpiceFileTransferTask
> > > > Flus
> > > > hCb flush_callback,
> > > >                                                      gpointer
> > > > flush_callback_data,
> > > >                                                      GAsyncReadyCallback
> > > > callback,
> > > > @@ -103,8 +101,6 @@ struct _SpiceFileTransferTask
> > > >      GFileInputStream               *file_stream;
> > > >      GFileCopyFlags                 flags;
> > > >      GCancellable                   *cancellable;
> > > > -    GFileProgressCallback          progress_callback;
> > > > -    gpointer                       progress_callback_data;
> > > >      SpiceFileTransferTaskFlushCb   flush_callback;
> > > >      gpointer                       flush_callback_data;
> > > >      GAsyncReadyCallback            callback;
> > > > @@ -156,6 +152,15 @@ typedef struct {
> > > >      SpiceDisplayState       display_state;
> > > >  } SpiceDisplayConfig;
> > > >  
> > > > +typedef struct {
> > > > +    GList                      *tasks;
> > > > +    SpiceMainChannel           *channel;
> > > > +    GFileProgressCallback       progress_callback;
> > > > +    gpointer                    progress_callback_data;
> > > > +    goffset                     total_sent;
> > > > +    goffset                     transfer_size;
> > > > +} FileTransferOperation;
> > > > +
> > > >  struct _SpiceMainChannelPrivate  {
> > > >      enum SpiceMouseMode         mouse_mode;
> > > >      enum SpiceMouseMode         requested_mouse_mode;
> > > > @@ -257,6 +262,13 @@ static void file_xfer_flushed(SpiceMainChannel
> > > > *channel,
> > > > gboolean success);
> > > >  static void spice_main_set_max_clipboard(SpiceMainChannel *self, gint
> > > > max);
> > > >  static void set_agent_connected(SpiceMainChannel *channel, gboolean
> > > > connected);
> > > >  
> > > > +static void file_transfer_operation_send_progress(SpiceMainChannel
> > > > *channel,
> > > > SpiceFileTransferTask *xfer_task);
> > > > +static void file_transfer_operation_end(FileTransferOperation *xfer_op);
> > > > +static void file_transfer_operation_reset_all(SpiceMainChannel *channel);
> > > > +static SpiceFileTransferTask
> > > > *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> > > > +                                                                      gui
> > > > nt32
> > > > task_id);
> > > > +static void file_transfer_operation_task_finished(SpiceMainChannel
> > > > *channel,
> > > > SpiceFileTransferTask *task);
> > > > +
> > > >  /* ------------------------------------------------------------------ */
> > > >  
> > > >  static const char *agent_msg_types[] = {
> > > > @@ -315,8 +327,7 @@ static void spice_main_channel_init(SpiceMainChannel
> > > > *channel)
> > > >  
> > > >      c = channel->priv = SPICE_MAIN_CHANNEL_GET_PRIVATE(channel);
> > > >      c->agent_msg_queue = g_queue_new();
> > > > -    c->file_xfer_tasks = g_hash_table_new_full(g_direct_hash,
> > > > g_direct_equal,
> > > > -                                               NULL, g_object_unref);
> > > > +    c->file_xfer_tasks = g_hash_table_new(g_direct_hash, g_direct_equal);
> > > >      c->flushing = g_hash_table_new_full(g_direct_hash, g_direct_equal,
> > > > NULL,
> > > >                                          g_object_unref);
> > > >      c->cancellable_volume_info = g_cancellable_new();
> > > > @@ -470,9 +481,6 @@ static void spice_channel_iterate_write(SpiceChannel
> > > > *channel)
> > > >  static void spice_main_channel_reset_agent(SpiceMainChannel *channel)
> > > >  {
> > > >      SpiceMainChannelPrivate *c = channel->priv;
> > > > -    GError *error;
> > > > -    GList *tasks;
> > > > -    GList *l;
> > > >  
> > > >      c->agent_connected = FALSE;
> > > >      c->agent_caps_received = FALSE;
> > > > @@ -481,15 +489,7 @@ static void
> > > > spice_main_channel_reset_agent(SpiceMainChannel *channel)
> > > >      g_clear_pointer(&c->agent_msg_data, g_free);
> > > >      c->agent_msg_size = 0;
> > > >  
> > > > -    tasks = g_hash_table_get_values(c->file_xfer_tasks);
> > > > -    for (l = tasks; l != NULL; l = l->next) {
> > > > -        SpiceFileTransferTask *task = (SpiceFileTransferTask *)l->data;
> > > > -
> > > > -        error = g_error_new(SPICE_CLIENT_ERROR,
> > > > SPICE_CLIENT_ERROR_FAILED,
> > > > -                            "Agent connection closed");
> > > > -        spice_file_transfer_task_completed(task, error);
> > > > -    }
> > > > -    g_list_free(tasks);
> > > > +    file_transfer_operation_reset_all(channel);
> > > >      file_xfer_flushed(channel, FALSE);
> > > >  }
> > > >  
> > > > @@ -1878,7 +1878,9 @@ static void file_xfer_data_flushed_cb(GObject
> > > > *source_object,
> > > >      SpiceMainChannel *channel = (SpiceMainChannel *)source_object;
> > > >      GError *error = NULL;
> > > >  
> > > > -    file_xfer_flush_finish(channel, res, &error);
> > > > +    if (file_xfer_flush_finish(channel, res, &error))
> > > > +        file_transfer_operation_send_progress(channel, self);
> > > > +
> > > Slight change of behavior here. Perviously it seems that we sent the
> > > progress
> > > even if flush_finish was not successful. I don't know what the right
> > > behavior
> > > is. I assume this was intentional?
> > It was but I don't recall exactly the issue that I had. Overall, I think
> > it makes sense to send progress only in case some progress were made
> > instead of failures.
> > 
> > > 
> > > 
> > > > 
> > > >      spice_file_transfer_task_flush_done(self, error);
> > > >  }
> > > >  
> > > > @@ -1905,7 +1907,9 @@ static void
> > > > file_xfer_flush_callback(SpiceFileTransferTask *xfer_task,
> > > >  {
> > > >      SpiceMainChannel *main_channel;
> > > >      GCancellable *cancellable;
> > > > +    FileTransferOperation *xfer_op = user_data;
> > > >  
> > > > +    xfer_op->total_sent += count;
> > > >      file_xfer_queue(xfer_task, buffer, count);
> > > >      if (count == 0)
> > > >          return;
> > > > @@ -2146,7 +2150,7 @@ static void main_agent_handle_msg(SpiceChannel
> > > > *channel,
> > > >          SpiceFileTransferTask *task;
> > > >          VDAgentFileXferStatusMessage *msg = payload;
> > > >  
> > > > -        task = g_hash_table_lookup(c->file_xfer_tasks,
> > > > GUINT_TO_POINTER(msg-
> > > > > 
> > > > > id));
> > > > +        task = file_transfer_operation_find_task_by_id(self, msg->id);
> > > >          if (task != NULL) {
> > > >              spice_file_transfer_task_handle_status(task, msg);
> > > >          } else {
> > > > @@ -3042,21 +3046,22 @@ static void
> > > > file_xfer_on_file_info(SpiceFileTransferTask *xfer_task,
> > > >                                     GFileInfo *info,
> > > >                                     gpointer data)
> > > >  {
> > > > -    SpiceMainChannel *channel;
> > > >      GKeyFile *keyfile;
> > > >      VDAgentFileXferStartMessage msg;
> > > >      gchar *string, *basename;
> > > >      guint64 file_size;
> > > >      gsize data_len;
> > > >      GError *error = NULL;
> > > > +    FileTransferOperation *xfer_op;
> > > >  
> > > >      g_return_if_fail(info != NULL);
> > > > -
> > > > -    channel = SPICE_MAIN_CHANNEL(data);
> > > > +    xfer_op = data;
> > > >  
> > > >      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->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);
> > > > @@ -3072,34 +3077,122 @@ static void
> > > > file_xfer_on_file_info(SpiceFileTransferTask *xfer_task,
> > > >  
> > > >      /* Create file-xfer start message */
> > > >      msg.id = spice_file_transfer_task_get_id(xfer_task);
> > > > -    agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_START,
> > > > +    agent_msg_queue_many(xfer_op->channel, VD_AGENT_FILE_XFER_START,
> > > >                           &msg, sizeof(msg),
> > > >                           string, data_len + 1, NULL);
> > > >      g_free(string);
> > > > -    spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE);
> > > > +    spice_channel_wakeup(SPICE_CHANNEL(xfer_op->channel), FALSE);
> > > >      return;
> > > >  
> > > >  failed:
> > > >      spice_file_transfer_task_completed(xfer_task, error);
> > > >  }
> > > >  
> > > > +static void file_transfer_operation_send_progress(SpiceMainChannel
> > > > *channel,
> > > > SpiceFileTransferTask *xfer_task)
> > > > +{
> > > > +    FileTransferOperation *xfer_op;
> > > > +    guint32 task_id;
> > > > +
> > > > +    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->total_sent,
> > > > +                                   xfer_op->transfer_size,
> > > > +                                   xfer_op->progress_callback_data);
> > > > +}
> > > > +
> > > > +static void file_transfer_operation_end(FileTransferOperation *xfer_op)
> > > > +{
> > > > +    g_return_if_fail(xfer_op != NULL);
> > > > +    spice_debug("Freeing file-transfer-operation %p", xfer_op);
> > > > +
> > > > +    /* SpiceFileTransferTask itself is freed after it emits "finish" */
> > > > +    if (xfer_op->tasks != NULL)
> > > > +        g_list_free(xfer_op->tasks);
> > > Is there any situation where we would expect tasks to be non-NULL?
> > > Should we at least print a warning for this case?
> > Agreed as it would leak the list pointer, I'll change it.
> > 
> > > 
> > > 
> > > > 
> > > > +
> > > > +    g_free(xfer_op);
> > > > +}
> > > > +
> > > > +static void file_transfer_operation_reset_all(SpiceMainChannel *channel)
> > > > +{
> > > > +    GList *list_op, *it_op;
> > > > +
> > > > +    list_op = g_hash_table_get_values(channel->priv->file_xfer_tasks);
> > > > +    for (it_op = list_op; it_op != NULL; it_op = it_op->next) {
> > > > +        FileTransferOperation *op = it_op->data;
> > > > +        GList *it;
> > > > +        for (it = op->tasks; it != NULL; it = it->next) {
> > > > +            SpiceFileTransferTask *xfer_task =
> > > > SPICE_FILE_TRANSFER_TASK(it-
> > > > > 
> > > > > data);
> > > > +            GError *error = g_error_new(SPICE_CLIENT_ERROR,
> > > > SPICE_CLIENT_ERROR_FAILED,
> > > > +                                        "Agent connection closed");
> > > > +            spice_file_transfer_task_completed(xfer_task, error);
> > > > +        }
> > > Since the operation is present in the hash table once for each task that it
> > > contains, it seems that above you will be completing the same task multiple
> > > times for these multi-task operations.
> > > 
> > > For example, if Operation A has 4 tasks, g_hash_table_get_values() will
> > > return a
> > > list of 4 items, all containing operation A. So I think you'll call
> > > spice_file_transfer_task_completed() for each of the tasks in operation A
> > > four
> > > times. 
> > That's true.. I got myself confused a few times already here due the
> > list_op provided by g_hash_table_get_values on file_xfer_tasks. I think
> > we can improve that based in the comment below.
> > 
> > > 
> > > 
> > > > 
> > > > +    }
> > > > +    g_list_free(list_op);
> > > > +}
> > > > +
> > > > +static SpiceFileTransferTask
> > > > *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> > > > +                                                                      gui
> > > > nt32
> > > > task_id)
> > > > +{
> > > > +    FileTransferOperation *op;
> > > > +    GList *it;
> > > > +
> > > > +    op = g_hash_table_lookup (channel->priv->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id));
> > > > +    g_return_val_if_fail (op != NULL, NULL);
> > > > +
> > > > +    for (it = op->tasks; it != NULL; it = it->next) {
> > > > +        SpiceFileTransferTask *xfer_task = SPICE_FILE_TRANSFER_TASK(it-
> > > > > 
> > > > > data);
> > > > +        guint32 id = spice_file_transfer_task_get_id(xfer_task);
> > > > +        if (id == task_id)
> > > > +            return xfer_task;
> > > > +    }
> > > > +    return NULL;
> > > > +}
> > > This feels a bit convoluted to me. There is a hash table that relates
> > > a task ID to a FileTransferOperation. The FileTransferOperation has a
> > > list of tasks, and each task has an ID. And since the Operations are
> > > inserted into the hash table once for each task they contain, you may
> > > iterate over the same operation multiple times before you get to the
> > > operation that contains your task. For example, consider where you
> > > have two operations: OperationA with 10 tasks, Operation B with only
> > > 1. And say you're looking for the task in operation B. You could end
> > > up iterating through the ten tasks of operation A ten times (100 total
> > > iterations) before you get to the task you want.
> > Unless I'm missing something, I don't think this is correct. On the code
> > above, we get the correct FileTransferOperation on g_hash_table_lookup
> > (so, in your example, that would be operation B) and the for below would
> > interact only once to find the correct SpiceFileTransferTask;
> 
> Yes, you're correct. I clearly read through this code too quickly. 
> 
> > 
> > If the example was about how much we interact in a big
> > FileTransferOperation, for instance, with 100 items, that would be bad
> > indeed and in this case we might consider moving away from GList to
> > GHashTable in the FileTransferOperation. If we agree with this, I would
> > change the _create_tasks() function to return a GHashTable instead of
> > GList.
> > 
> > > 
> > > Granted, this isn't a performance-critical path, but considering the
> > > other issues mentioned above, I feel there's probably a better design.
> > Indeed, it could be better, what do you think about the following:
> > 
> > * channel->priv->file_xfer_tasks: (GHashTable) that provides mapping
> >   from task_id to FileTransferOperation ~ useful to get the correct
> >   xfer_op based on task_id which is what we receive from Agent
> > 
> > * xfer_op->tasks: (GHashTable) that provides mapping from task_id to the
> >   actual FileTransferTask pointer ~ This should makes us not worry about
> >   performance in large tasks sets.
> > 
> > > 
> > > 
> > > > 
> > > > +
> > > > +static void file_transfer_operation_task_finished(SpiceMainChannel
> > > > *channel,
> > > > SpiceFileTransferTask *task)
> > > > +{
> > > > +    FileTransferOperation *xfer_op;
> > > > +    guint32 task_id;
> > > > +
> > > > +    task_id = spice_file_transfer_task_get_id(task);
> > > > +    xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id));
> > > > +    if (xfer_op == NULL) {
> > > > +        /* Likely the operation has ended before the remove-task was
> > > > called.
> > > > One
> > > > +         * situation that this can easily happen is if the agent is
> > > > disconnected
> > > > +         * while there are pending files. */
> > > > +        g_object_unref(task);
> > > > +        return;
> > > > +    }
> > > > +    /* Remove and free SpiceFileTransferTask */
> > > > +    xfer_op->tasks = g_list_remove(xfer_op->tasks, task);
> > > > +    g_object_unref(task);
> > > > +
> > > > +    /* Keep file-xfer-tasks up to date. If no more elements, operation is
> > > > over */
> > > > +    g_hash_table_remove(channel->priv->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id));
> > > > +
> > > > +    /* No more pending operations */
> > > > +    if (xfer_op->tasks == NULL)
> > > > +        file_transfer_operation_end(xfer_op);
> > > > +}
> > > > +
> > > >  static void task_finished(SpiceFileTransferTask *task,
> > > >                            GError *error,
> > > >                            gpointer data)
> > > >  {
> > > >      SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
> > > > -    guint32 task_id = spice_file_transfer_task_get_id(task);
> > > >  
> > > >      if (error) {
> > > >          VDAgentFileXferStatusMessage msg;
> > > > -        msg.id = task_id;
> > > > +        msg.id = spice_file_transfer_task_get_id(task);
> > > >          msg.result = error->code == G_IO_ERROR_CANCELLED ?
> > > >                  VD_AGENT_FILE_XFER_STATUS_CANCELLED :
> > > > VD_AGENT_FILE_XFER_STATUS_ERROR;
> > > >          agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_STATUS,
> > > >                               &msg, sizeof(msg), NULL);
> > > >      }
> > > >  
> > > > -    g_hash_table_remove(channel->priv->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id));
> > > > +    file_transfer_operation_task_finished(channel, task);
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -3145,7 +3238,8 @@ void spice_main_file_copy_async(SpiceMainChannel
> > > > *channel,
> > > >                                  gpointer user_data)
> > > >  {
> > > >      SpiceMainChannelPrivate *c = channel->priv;
> > > > -    GList *tasks, *it;
> > > > +    GList *it;
> > > > +    FileTransferOperation *xfer_op;
> > > >  
> > > >      g_return_if_fail(channel != NULL);
> > > >      g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> > > > @@ -3162,27 +3256,29 @@ void spice_main_file_copy_async(SpiceMainChannel
> > > > *channel,
> > > >          return;
> > > >      }
> > > >  
> > > > -    tasks = spice_file_transfer_task_create_tasks(channel,
> > > > -                                                  sources,
> > > > -                                                  flags,
> > > > -                                                  cancellable,
> > > > -                                                  progress_callback,
> > > > -                                                  progress_callback_data,
> > > > -                                                  file_xfer_flush_callbac
> > > > k,
> > > > -                                                  NULL,
> > > > -                                                  callback,
> > > > -                                                  user_data);
> > > > -    for (it = tasks; it != NULL; it = it->next) {
> > > > +    xfer_op = g_new0(FileTransferOperation, 1);
> > > > +    xfer_op->progress_callback = progress_callback;
> > > > +    xfer_op->progress_callback_data = progress_callback_data;
> > > > +    xfer_op->channel = channel;
> > > > +    xfer_op->tasks = spice_file_transfer_task_create_tasks(channel,
> > > > +                                                           sources,
> > > > +                                                           flags,
> > > > +                                                           cancellable,
> > > > +                                                           file_xfer_flus
> > > > h_ca
> > > > llback,
> > > > +                                                           xfer_op,
> > > > +                                                           callback,
> > > > +                                                           user_data);
> > > What about changing 
> > > 
> > > GList *spice_file_transfer_task_create_tasks()
> > > 
> > > to
> > > 
> > > FileTransferOperation spice_file_transfer_operation_create()?
> > I would agree to change the return value to GHashTable but not to
> > FileTransferOperation structure as this is the wrapper from
> > spice_main_file_copy_async data so it makes sense to me to keep this
> > struct internal to channel-main.
> 
> Well, perhaps this function itself could be local to channel-main since it's
> basically just wrapping spice_file_transfer_task_new(). On the other hand, I
> don't necessarily think that the FileTransferOperation needs to be internal to
> channel-main. But I get your point.
> 
> 
> > 
> > I don't mind about changing the function name but maybe
> > spice_file_transfer_task_operation_create() instead or just change to
> > singular the previous one, spice_file_transfer_task_create_task as I
> > often associate plural to lists but not to hashtables.
> > 
> > > 
> > > 
> > > 
> > > > 
> > > > +    spice_debug("New file-transfer-operation %p", xfer_op);
> > > > +    for (it = xfer_op->tasks; it != NULL; it = it->next) {
> > > >          SpiceFileTransferTask *task = SPICE_FILE_TRANSFER_TASK(it->data);
> > > >          guint32 task_id = spice_file_transfer_task_get_id(task);
> > > >  
> > > > -        g_hash_table_insert(c->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id),
> > > > task);
> > > > -        g_signal_connect(task, "file-info",
> > > > G_CALLBACK(file_xfer_on_file_info), channel);
> > > > +        g_hash_table_insert(c->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id),
> > > > xfer_op);
> > > > +        g_signal_connect(task, "file-info",
> > > > G_CALLBACK(file_xfer_on_file_info), xfer_op);
> > > >          g_signal_connect(task, "finished", G_CALLBACK(task_finished),
> > > > channel);
> > > >          g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> > > > task);
> > > >          spice_file_transfer_task_start_task(task);
> > > >      }
> > > > -    g_list_free(tasks);
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -3251,26 +3347,6 @@ static void
> > > > spice_file_transfer_task_flush_done(SpiceFileTransferTask *self, GEr
> > > >          }
> > > >      }
> > > >  
> > > > -    if (self->progress_callback) {
> > > > -        goffset read = 0;
> > > > -        goffset total = 0;
> > > > -        SpiceMainChannel *main_channel = self->channel;
> > > > -        GHashTableIter iter;
> > > > -        gpointer key, value;
> > > > -
> > > > -        /* 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, main_channel->priv-
> > > > >file_xfer_tasks);
> > > > -        while (g_hash_table_iter_next(&iter, &key, &value)) {
> > > > -            SpiceFileTransferTask *t = (SpiceFileTransferTask *)value;
> > > > -            read += t->read_bytes;
> > > > -            total += t->file_size;
> > > > -        }
> > > > -
> > > > -        self->progress_callback(read, total, self-
> > > > >progress_callback_data);
> > > > -    }
> > > > -
> > > >      /* Read more data */
> > > >      file_xfer_continue_read(self);
> > > >  }
> > > > @@ -3279,8 +3355,6 @@ static GList
> > > > *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> > > >                                                      GFile **files,
> > > >                                                      GFileCopyFlags flags,
> > > >                                                      GCancellable
> > > > *cancellable,
> > > > -                                                    GFileProgressCallback
> > > > progress_callback,
> > > > -                                                    gpointer
> > > > progress_callback_data,
> > > >                                                      SpiceFileTransferTask
> > > > Flus
> > > > hCb flush_callback,
> > > >                                                      gpointer
> > > > flush_callback_data,
> > > >                                                      GAsyncReadyCallback
> > > > callback,
> > > > @@ -3300,8 +3374,6 @@ static GList
> > > > *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> > > >  
> > > >          task = spice_file_transfer_task_new(channel, files[i],
> > > > task_cancellable);
> > > >          task->flags = flags;
> > > > -        task->progress_callback = progress_callback;
> > > > -        task->progress_callback_data = progress_callback_data;
> > > >          task->flush_callback = flush_callback;
> > > >          task->flush_callback_data = flush_callback_data;
> > > >          task->callback = callback;
> > > 
> > > Perhaps it would be simpler to simply keep the hash table as a map between
> > > task
> > > ID and SpiceFileTransferTask, but add a field to SpiceFileTransferTask
> > > pointing
> > > to its operation?
> > That would do as well! (sorry, I should have read the whole email before
> > starting to reply).
> 
> To be fair, I didn't write my review in the best order either. New bits of code
> triggered new ideas, so my thinking changed slightly over the course of the
> review ;)
> 
> > 
> > So, with my proposal, we would do:
> > 1a) from task_id gets FileTransferOperation (xfer_op) from
> >     channel->priv->file_xfer_tasks
> > 2a) from task_id gets SpiceFileTransferTask (xfer_task) from xfer_op's
> >     GHashTable
> > 
> > With your proposal, we would do:
> > 1b) from task_id gets SpiceFileTransferTask (xfer_task) from
> >     channel->priv->file_xfer_tasks
> > 2b) from xfer_task gets xfer_op
> > 
> > I still prefer my proposal because we wouldn't need to share
> > FileTransferOperation logic with SpiceFileTransferTask.
> 
> It's true that my suggestion does allow the FileTransferOperation to bleed over
> into SpiceFileTransferTask. On the other hand, it does make a few things much
> simpler and more straightforward (as mentioned above). So I think I would
> personally prefer to expose the FileTansferOperation to SpiceFileTransferTask if
> it makes the rest of the implementation cleaner. But that's mostly a personal
> preference.

As there is a lot of things going on, I'll do the next version with my
proposal (mostly because it is easier for me) and we can re-evaluate
then if we should change.

Cheers,
  toso

>
> >
> > >
> > >
> > > Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > Let me know what you think!
> >   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]