Hi,
On Fri, Jun 24, 2016 at 04:45:51PM -0500, Jonathon Jongsma wrote:
> On Thu, 2016-06-23 at 19:37 +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:
> >
> > * file_transfer_operation_end
> > * file_transfer_operation_reset_all
> > * file_transfer_operation_find_task_by_id
> > * 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 | 150 ++++++++++++++++++++++++++++++++++++++------------
> > ---
> > 1 file changed, 109 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 5e23acb..689b709 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -159,6 +159,11 @@ typedef struct {
> > SpiceDisplayState display_state;
> > } SpiceDisplayConfig;
> >
> > +typedef struct {
> > + GHashTable *xfer_task_ht;
>
> Personally, I prefer not to embed the type into the name. Something like
> 'xfer_tasks' perhaps.
Sure
>
> > + SpiceMainChannel *channel;
> > +} FileTransferOperation;
> > +
> > struct _SpiceMainChannelPrivate {
> > enum SpiceMouseMode mouse_mode;
> > enum SpiceMouseMode requested_mouse_mode;
> > @@ -262,6 +267,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_end(FileTransferOperation *xfer_op);
> > +static void file_transfer_operation_reset_all(SpiceMainChannel *channel);
>
> this appears to be a SpiceMainChannel method instead of a FileTransferOperation
> method, so i'd prefer the name to reflect that. e.g.
> spice_main_channel_reset_all_xfer_operations()? Maybe there's better options.
Makes sense
>
> > +static SpiceFileTransferTask
> > *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> > + guint32
> > task_id);
>
> Same here.
I'll change as well
>
> > +static void file_transfer_operation_task_finished(SpiceFileTransferTask
> > *xfer_task,
> > + GError *error,
> > + gpointer userdata);
> > +
> > /* ------------------------------------------------------------------ */
> >
> > static const char *agent_msg_types[] = {
> > @@ -320,10 +333,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));
> > @@ -475,9 +486,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;
> > @@ -486,15 +494,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);
> > }
> >
> > @@ -1899,12 +1899,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 */
> > /* 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 = file_transfer_operation_find_task_by_id(channel,
> > GPOINTER_TO_UINT(key));
> > read += t->read_bytes;
> > total += t->file_size;
> > }
> > @@ -1997,11 +2002,9 @@ static void file_xfer_handle_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 = file_transfer_operation_find_task_by_id(channel, msg->id);
> > g_return_if_fail(xfer_task != NULL);
> >
> > SPICE_DEBUG("xfer-task %u received response %u", msg->id, msg->result);
> > @@ -3063,18 +3066,82 @@ 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_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" */
> > + g_hash_table_unref(xfer_op->xfer_task_ht);
> > + g_free(xfer_op);
> > +}
>
> Not sure about this name. the _end() name doesn't necessarily give the
> impression that xfer_op cannot be used after calling this function. I
> think _free() would be more clear.
I agree
>
> >
> > -static void task_finished(SpiceFileTransferTask *task,
> > - GError *error,
> > - gpointer data)
> > +static void file_transfer_operation_reset_all(SpiceMainChannel *channel)
> > {
> > - SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
> > - g_hash_table_remove(channel->priv->file_xfer_tasks,
> > GUINT_TO_POINTER(task->id));
> > + GHashTableIter iter_all_xfer_tasks;
> > + gpointer key, value;
> > +
> > + /* Mark each of SpiceFileTransferTask as completed due error */
>
> "due to an 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_ht, key);
> > + GError *error;
> > +
> > + if (xfer_task == NULL)
> > + continue;
>
> expected? maybe print a warning here?
It is not expected, I played in the safe side here. Warning would be
good indeed
>
> > +
> > + error = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > + "Agent connection closed");
> > + spice_file_transfer_task_completed(xfer_task, error);
> > + }
> > +}
> > +
> > +static SpiceFileTransferTask
> > *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> > + guint32
> > 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_ht,
> > 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);
>
> This is not related to this change particularly, but I'm suddenly wondering
> whether we should really have this reverse dependency from SpiceFileTransferTask
> back to SpiceMainChannel... It would require a fair amount of re-design to get
> rid of it though. Just thinking out loud...
Yeah, actually, we should avoid using _get_channel() and use
xfer_op->channel instead.
The rationale is that we should (in channel-main) depend on xfer_op for
those things. I'll change it in this patch.
We should be able to remove the _get_channel() helper in the future.
>
> > + 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));
> > + 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_ht, GUINT_TO_POINTER(task_id));
> > + g_object_unref(xfer_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 (g_hash_table_size(xfer_op->xfer_task_ht) == 0)
> > + file_transfer_operation_end(xfer_op);
> > }
> >
> > +static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel
> > *channel,
> > + GFile *file,
> > + GCancellable
> > *cancellable);
> > +
> > /**
> > * spice_main_file_copy_async:
> > * @channel: a #SpiceMainChannel
> > @@ -3118,9 +3185,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));
> > @@ -3138,15 +3205,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_ht = spice_file_transfer_task_create_tasks(sources,
> > + channel,
> > + flags,
> > + cancellable
> > ,
> > + progress_ca
> > llback,
> > + progress_ca
> > llback_data,
> > + callback,
> > + user_data);
> > + g_hash_table_iter_init(&iter, xfer_op->xfer_task_ht);
> > while (g_hash_table_iter_next(&iter, &key, &value)) {
> > guint32 task_id;
> > SpiceFileTransferTask *xfer_task;
> > @@ -3156,15 +3225,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);
> > }
> >
> > /**
>
>
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
Thanks,
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