Re: [spice-gtk v5 11/23] file-xfer: a FileTransferOperation per transfer call

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

 



Hi,

On Wed, Jul 06, 2016 at 11:01:11AM -0500, Jonathon Jongsma wrote:
> On Tue, 2016-07-05 at 15:07 +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 in a GHashTable.
> > 
> > To ease the review process, this first patch introduces the structure
> > and organize the logic around the four following helpers:
> > 
> > * spice_main_channel_reset_all_xfer_operations
> > * spice_main_channel_find_xfer_task_by_task_id
> > * file_transfer_operation_free
> > * file_transfer_operation_task_finished
> > 
> > The _task_finished function is the new callback for "finished" signal
> > from SpiceFileTransferTask.
> > 
> > The file_xfer_tasks GHashTable is now mapped as:
> >  (key) SpiceFileTransferTask ID -> (value) FileTransferOperation
> > 
> > This patch leaves a FIXME regarding progress_callback which will be
> > addressed in a later patch in this series.
> > 
> > This change is related to split SpiceFileTransferTask from
> > channel-main.
> > ---
> >  src/channel-main.c | 168 ++++++++++++++++++++++++++++++++++++++++----------
> > ---
> >  1 file changed, 127 insertions(+), 41 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 529c36c..8e8f6bd 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -158,6 +158,11 @@ typedef struct {
> >      SpiceDisplayState       display_state;
> >  } SpiceDisplayConfig;
> >  
> > +typedef struct {
> > +    GHashTable                 *xfer_task;
> > +    SpiceMainChannel           *channel;
> > +} FileTransferOperation;
> > +
> >  struct _SpiceMainChannelPrivate  {
> >      enum SpiceMouseMode         mouse_mode;
> >      enum SpiceMouseMode         requested_mouse_mode;
> > @@ -261,6 +266,14 @@ static void file_xfer_read_async_cb(GObject
> > *source_object,
> >  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_free(FileTransferOperation *xfer_op);
> > +static void spice_main_channel_reset_all_xfer_operations(SpiceMainChannel
> > *channel);
> > +static SpiceFileTransferTask
> > *spice_main_channel_find_xfer_task_by_task_id(SpiceMainChannel *channel,
> > +                                                                           gu
> > int32 task_id);
> > +static void file_transfer_operation_task_finished(SpiceFileTransferTask
> > *xfer_task,
> > +                                                  GError *error,
> > +                                                  gpointer userdata);
> > +
> >  /* ------------------------------------------------------------------ */
> >  
> >  static const char *agent_msg_types[] = {
> > @@ -319,10 +332,8 @@ 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->flushing = 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(g_direct_hash, g_direct_equal);
> >      c->cancellable_volume_info = g_cancellable_new();
> >  
> >      spice_main_channel_reset_capabilties(SPICE_CHANNEL(channel));
> > @@ -474,9 +485,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;
> > @@ -485,15 +493,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);
> > +    spice_main_channel_reset_all_xfer_operations(channel);
> >      file_xfer_flushed(channel, FALSE);
> >  }
> >  
> > @@ -1898,12 +1898,17 @@ static void
> > file_xfer_send_progress(SpiceFileTransferTask *xfer_task)
> >  
> >      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 */
>
> A couple recommendations here:
>  - "iterates to" should be "iterates over"
>  - "ones from different FileTransferOperation" should be "ones from
>  *a* different..." or "ones from different FileTransferOperation*s*"

Thanks! I'll fix them

> I assume that you closed the comment here since it's a FIXME that will
> be removed soon. It's a little weird to have two comments right in a
> row instead of one larger comment, but I guess that's fine.

I don't think I payed attention to this but I'll fix it as well, why not
:)

>
> >      /* 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 = (SpiceFileTransferTask *)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;
> >      }
> > @@ -1998,11 +2003,9 @@ static void
> > main_agent_handle_xfer_status(SpiceMainChannel *channel,
> >                                            VDAgentFileXferStatusMessage *msg)
> >  {
> >      SpiceFileTransferTask *xfer_task;
> > -    SpiceMainChannelPrivate *c;
> >      GError *error = NULL;
> >  
> > -    c = channel->priv;
> > -    xfer_task = g_hash_table_lookup(c->file_xfer_tasks, GUINT_TO_POINTER(msg-
> > >id));
> > +    xfer_task = spice_main_channel_find_xfer_task_by_task_id(channel, msg-
> > >id);
> >      g_return_if_fail(xfer_task != NULL);
> >  
> >      SPICE_DEBUG("xfer-task %u received response %u", msg->id, msg->result);
> > @@ -3073,18 +3076,100 @@ failed:
> >      spice_file_transfer_task_completed(xfer_task, error);
> >  }
> >  
> > -static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel
> > *channel,
> > -                                                           GFile *file,
> > -                                                           GCancellable
> > *cancellable);
> > +static void file_transfer_operation_free(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" */
> > +    g_hash_table_unref(xfer_op->xfer_task);
> > +    g_free(xfer_op);
> > +}
> > +
> > +static void spice_main_channel_reset_all_xfer_operations(SpiceMainChannel
> > *channel)
> > +{
> > +    GHashTableIter iter_all_xfer_tasks;
> > +    gpointer key, value;
> > +
> > +    /* Mark each of SpiceFileTransferTask as completed due error */
> > +    g_hash_table_iter_init(&iter_all_xfer_tasks, channel->priv-
> > >file_xfer_tasks);
> > +    while (g_hash_table_iter_next(&iter_all_xfer_tasks, &key, &value)) {
> > +        FileTransferOperation *xfer_op = value;
> > +        SpiceFileTransferTask *xfer_task = g_hash_table_lookup(xfer_op-
> > >xfer_task, key);
> > +        GError *error;
> > +
> > +        if (xfer_task == NULL) {
> > +            spice_warning("(reset-all) can't complete task %u - completed
> > already?",
> > +                          GPOINTER_TO_UINT(key));
> > +            continue;
> > +        }
> > +
> > +        error = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > +                            "Agent connection closed");
> > +        spice_file_transfer_task_completed(xfer_task, error);
> > +    }
> > +}
> >  
> > -static void task_finished(SpiceFileTransferTask *task,
> > -                          GError *error,
> > -                          gpointer data)
> > +static SpiceFileTransferTask
> > *spice_main_channel_find_xfer_task_by_task_id(SpiceMainChannel *channel,
> > +                                                                           gu
> > int32 task_id)
> >  {
> > -    SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
> > -    g_hash_table_remove(channel->priv->file_xfer_tasks,
> > GUINT_TO_POINTER(task->id));
> > +    FileTransferOperation *xfer_op;
> > +
> > +    xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks,
> > GUINT_TO_POINTER(task_id));
> > +    g_return_val_if_fail(xfer_op != NULL, NULL);
> > +    return g_hash_table_lookup(xfer_op->xfer_task,
> > GUINT_TO_POINTER(task_id));
> >  }
> >  
> > +static void file_transfer_operation_task_finished(SpiceFileTransferTask
> > *xfer_task,
> > +                                                  GError *error,
> > +                                                  gpointer userdata)
> > +{
> > +    SpiceMainChannel *channel;
> > +    FileTransferOperation *xfer_op;
> > +    guint32 task_id;
> > +
> > +    channel = spice_file_transfer_task_get_channel(xfer_task);
> > +    g_return_if_fail(channel != NULL);
> > +    task_id = spice_file_transfer_task_get_id(xfer_task);
> > +    g_return_if_fail(task_id != 0);
> > +
> > +    if (error) {
> > +        VDAgentFileXferStatusMessage msg;
> > +        msg.id = task_id;
> > +        if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
> > +            msg.result = VD_AGENT_FILE_XFER_STATUS_CANCELLED;
> > +        } else {
> > +            msg.result = VD_AGENT_FILE_XFER_STATUS_ERROR;
> > +        }
> > +        agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_STATUS,
> > +                             &msg, sizeof(msg), NULL);
> > +    }
> > +
> > +    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(xfer_task);
> > +        return;
> > +    }
> > +
> > +    /* Remove and free SpiceFileTransferTask */
> > +    g_hash_table_remove(xfer_op->xfer_task, GUINT_TO_POINTER(task_id));
> > +    g_object_unref(xfer_task);
> > +
> > +    /* Keep file-xfer-tasks up to date. If no more elements, operation is
> > over */
> 
> file-xfer-tasks -> file_xfer_tasks

Fixed

> 
> > +    g_hash_table_remove(channel->priv->file_xfer_tasks,
> > GUINT_TO_POINTER(task_id));
> > +
> > +    /* No more pending operations */
> > +    if (g_hash_table_size(xfer_op->xfer_task) == 0)
> > +        file_transfer_operation_free(xfer_op);
> > +}
> > +
> > +static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel
> > *channel,
> > +                                                           GFile *file,
> > +                                                           GCancellable
> > *cancellable);
> > +
> >  /**
> >   * spice_main_file_copy_async:
> >   * @channel: a #SpiceMainChannel
> > @@ -3128,9 +3213,9 @@ void spice_main_file_copy_async(SpiceMainChannel
> > *channel,
> >                                  gpointer user_data)
> >  {
> >      SpiceMainChannelPrivate *c;
> > -    GHashTable *xfer_ht;
> >      GHashTableIter iter;
> >      gpointer key, value;
> > +    FileTransferOperation *xfer_op;
> >  
> >      g_return_if_fail(channel != NULL);
> >      g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> > @@ -3148,15 +3233,17 @@ void spice_main_file_copy_async(SpiceMainChannel
> > *channel,
> >          return;
> >      }
> >  
> > -    xfer_ht = spice_file_transfer_task_create_tasks(sources,
> > -                                                    channel,
> > -                                                    flags,
> > -                                                    cancellable,
> > -                                                    progress_callback,
> > -                                                    progress_callback_data,
> > -                                                    callback,
> > -                                                    user_data);
> > -    g_hash_table_iter_init(&iter, xfer_ht);
> > +    xfer_op = g_new0(FileTransferOperation, 1);
> > +    xfer_op->channel = channel;
> > +    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);
> >      while (g_hash_table_iter_next(&iter, &key, &value)) {
> >          guint32 task_id;
> >          SpiceFileTransferTask *xfer_task = value;
> > @@ -3165,15 +3252,14 @@ void spice_main_file_copy_async(SpiceMainChannel
> > *channel,
> >  
> >          SPICE_DEBUG("Insert a xfer task:%u to task list", task_id);
> >  
> > -        g_hash_table_insert(c->file_xfer_tasks, key,
> > g_object_ref(xfer_task));
> > -        g_signal_connect(xfer_task, "finished", G_CALLBACK(task_finished),
> > channel);
> > +        g_hash_table_insert(c->file_xfer_tasks, key, xfer_op);
> > +        g_signal_connect(xfer_task, "finished",
> > G_CALLBACK(file_transfer_operation_task_finished), NULL);
> >          g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> > xfer_task);
> >  
> >          spice_file_transfer_task_init_task_async(xfer_task,
> >                                                   file_xfer_init_task_async_cb
> > ,
> >                                                   NULL);
> >      }
> > -    g_hash_table_unref(xfer_ht);
> >  }
> >  
> >  /**
>
>
> aside from the comment issues above,
> 
> 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




[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]