[Spice-devel] [spice-gtk v1] channel-main: improve code to use FileTransferOperation

[Jonathon Jongsma]

On Sat, 2016-08-06 at 15:26 +0200, Victor Toso wrote:
> Since commit b26818d0e00666 we are grouping all transferred files per
> operation (drag and drop); That leaded to some design flaws in the
> code. The changes suggested in this patch are (in execution order):
> 
> * On spice_file_transfer_task_read_async()
> - Passing FileTransferOperation data so in the callback and in
> further
>   function calls, we don't need to look for this;
> 
> Note that FileTransferOperation is not freed while any of its
> SpiceFileTransferTask exists and by design they are never finished
> while on pending state, so file_xfer_read_async_cb() should have a
> valid FileTransferOperation pointer.
> 
> * On file_xfer_read_async_cb()
> - Removed unnecessary variables;
> 
> * On file_xfer_flush_async()
> - Using SpiceFileTransferTask as parameter which can retrieve the
>   SpiceMainChannel and the GCancellable;
> - Using SpiceFileTransferTask as source_object for GTask with
>   FileTransferOperation as user_data which is helpful for its
>   callback;
> 
> * On file_xfer_flush_finish() and file_xfer_data_flushed_cb()
> - Using SpiceFileTransferTask as parameter and source_object check
> for
>   GTask
> ---
> 
> Well, to be honest I thought we could benefit more with this change.
> Having the SpiceFileTransferTask on file_xfer_flush_async() had some
> code removal on file_xfer_read_async_cb() but not that much in the
> other places of the code.
> 
> All in all, it makes sense to me but let me know what you think.


Sure, I think it's a slightly improvement. 

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>


> 
> ---
>  src/channel-main.c | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 14ad6cf..3f91a43 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -889,15 +889,22 @@ static void file_xfer_flushed(SpiceMainChannel
> *channel, gboolean success)
>                                  GUINT_TO_POINTER(success));
>  }
>  
> -static void file_xfer_flush_async(SpiceMainChannel *channel,
> GCancellable *cancellable,
> -                                  GAsyncReadyCallback callback,
> gpointer user_data)
> +static void file_xfer_flush_async(SpiceFileTransferTask *xfer_task,
> +                                  GAsyncReadyCallback callback,
> +                                  gpointer user_data)
>  {
>      GTask *task;
> -    SpiceMainChannelPrivate *c = channel->priv;
> +    SpiceMainChannel *channel;
> +    SpiceMainChannelPrivate *c;
>      gboolean was_empty;
>  
> -    task = g_task_new(channel, cancellable, callback, user_data);
> +    channel = spice_file_transfer_task_get_channel(xfer_task);
> +    task = g_task_new(xfer_task,
> +                      spice_file_transfer_task_get_cancellable(xfer_
> task),
> +                      callback,
> +                      user_data);
>  
> +    c = channel->priv;
>      was_empty = g_queue_is_empty(c->agent_msg_queue);
>      if (was_empty) {
>          g_task_return_boolean(task, TRUE);
> @@ -909,12 +916,13 @@ static void
> file_xfer_flush_async(SpiceMainChannel *channel, GCancellable *cance
>      g_hash_table_insert(c->flushing, g_queue_peek_tail(c-
> >agent_msg_queue), task);
>  }
>  
> -static gboolean file_xfer_flush_finish(SpiceMainChannel *channel,
> GAsyncResult *result,
> +static gboolean file_xfer_flush_finish(SpiceFileTransferTask
> *xfer_task,
> +                                       GAsyncResult *result,
>                                         GError **error)
>  {
>      GTask *task = G_TASK(result);
>  
> -    g_return_val_if_fail(g_task_is_valid(result, channel), FALSE);
> +    g_return_val_if_fail(g_task_is_valid(result, xfer_task), FALSE);
>  
>      return g_task_propagate_boolean(task, error);
>  }
> @@ -1756,11 +1764,10 @@ static void file_xfer_data_flushed_cb(GObject
> *source_object,
>                                        GAsyncResult *res,
>                                        gpointer user_data)
>  {
> -    SpiceFileTransferTask *xfer_task = user_data;
> -    SpiceMainChannel *channel = (SpiceMainChannel *)source_object;
> +    SpiceFileTransferTask *xfer_task =
> SPICE_FILE_TRANSFER_TASK(source_object);
>      GError *error = NULL;
>  
> -    file_xfer_flush_finish(channel, res, &error);
> +    file_xfer_flush_finish(xfer_task, res, &error);
>      if (error) {
>          spice_file_transfer_task_completed(xfer_task, error);
>          return;
> @@ -1770,7 +1777,7 @@ static void file_xfer_data_flushed_cb(GObject
> *source_object,
>      if (!spice_file_transfer_task_is_completed(xfer_task)) {
>          file_transfer_operation_send_progress(xfer_task);
>          /* Read more data */
> -        spice_file_transfer_task_read_async(xfer_task,
> file_xfer_read_async_cb, NULL);
> +        spice_file_transfer_task_read_async(xfer_task,
> file_xfer_read_async_cb, user_data);
>      }
>  }
>  
> @@ -1801,11 +1808,10 @@ static void file_xfer_read_async_cb(GObject
> *source_object,
>      SpiceMainChannel *channel;
>      gssize count;
>      char *buffer;
> -    GCancellable *cancellable;
> -    guint32 task_id;
>      GError *error = NULL;
>  
>      xfer_task = SPICE_FILE_TRANSFER_TASK(source_object);
> +    xfer_op = user_data;
>  
>      channel = spice_file_transfer_task_get_channel(xfer_task);
>      count = spice_file_transfer_task_read_finish(xfer_task, res,
> &buffer, &error);
> @@ -1822,13 +1828,9 @@ static void file_xfer_read_async_cb(GObject
> *source_object,
>          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));
> -    g_return_if_fail(xfer_op != NULL);
>      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);
> +    file_xfer_flush_async(xfer_task, file_xfer_data_flushed_cb,
> xfer_op);
>  }
>  
>  /* coroutine context */
> @@ -1836,17 +1838,19 @@ static void
> main_agent_handle_xfer_status(SpiceMainChannel *channel,
>                                            VDAgentFileXferStatusMessa
> ge *msg)
>  {
>      SpiceFileTransferTask *xfer_task;
> +    FileTransferOperation *xfer_op;
>      GError *error = NULL;
>  
>      xfer_task =
> spice_main_channel_find_xfer_task_by_task_id(channel, msg->id);
>      g_return_if_fail(xfer_task != NULL);
> +    xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks,
> GUINT_TO_POINTER(msg->id));
>  
>      SPICE_DEBUG("xfer-task %u received response %u", msg->id, msg-
> >result);
>  
>      switch (msg->result) {
>      case VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA:
>          g_return_if_fail(spice_file_transfer_task_is_completed(xfer_
> task) == FALSE);
> -        spice_file_transfer_task_read_async(xfer_task,
> file_xfer_read_async_cb, NULL);
> +        spice_file_transfer_task_read_async(xfer_task,
> file_xfer_read_async_cb, xfer_op);
>          return;
>      case VD_AGENT_FILE_XFER_STATUS_CANCELLED:
>          error = g_error_new(SPICE_CLIENT_ERROR,
> SPICE_CLIENT_ERROR_FAILED,