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@xxxxxxxxxx>
>
> ---
> 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,
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel