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

[Victor Toso]

Hi,

On Tue, Aug 09, 2016 at 11:33:16AM -0500, Jonathon Jongsma wrote:
> 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>

Pushed!
https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=cbfcaa8c82b4e1633f86c52019b45f6f58d92b3d


> 
> 
> > 
> > ---
> >  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,