Hi,
On Tue, Jun 07, 2016 at 11:28:45AM -0500, Jonathon Jongsma wrote:
> On Sun, 2016-06-05 at 11:31 +0200, Victor Toso wrote:
> > On Fri, Jun 03, 2016 at 11:42:13AM -0500, Jonathon Jongsma wrote:
> > >
> > > On Mon, 2016-05-30 at 11:55 +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 and also the progress_callback passed from Application.
> > > >
> > > > As pointed in the fix 113093dd00a1cf10f6d3c3589b7589a184cec081, the
> > > > progress_callback should provide information of the whole transfer
> > > > operation; For that reason, there is no need to keep progress_callback
> > > > and progress_callback_data per SpiceFileTransferTask but per
> > > > FileTransferOperation.
> > > >
> > > > The file_xfer_tasks hash table now has FileTransferOperation instead
> > > > of SpiceFileTransferTask. To improve handling this new operation, I've
> > > > created the helpers:
> > > >
> > > > * file_transfer_operation_send_progress
> > > > * file_transfer_operation_end
> > > > * file_transfer_operation_reset_all
> > > > * file_transfer_operation_find_task_by_id
> > > > * file_transfer_operation_task_finished
> > > >
> > > > This change is related to split SpiceFileTransferTask from
> > > > channel-main.
> > > > ---
> > > > src/channel-main.c | 206 ++++++++++++++++++++++++++++++++++++----------
> > > > ----
> > > > ---
> > > > 1 file changed, 139 insertions(+), 67 deletions(-)
> > > >
> > > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > > index 72dcf1f..f36326d 100644
> > > > --- a/src/channel-main.c
> > > > +++ b/src/channel-main.c
> > > > @@ -66,8 +66,6 @@ static GList
> > > > *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> > > > GFile **files,
> > > > GFileCopyFlags flags,
> > > > GCancellable
> > > > *cancellable,
> > > > - GFileProgressCallback
> > > > progress_callback,
> > > > - gpointer
> > > > progress_callback_data,
> > > > SpiceFileTransferTask
> > > > Flus
> > > > hCb flush_callback,
> > > > gpointer
> > > > flush_callback_data,
> > > > GAsyncReadyCallback
> > > > callback,
> > > > @@ -103,8 +101,6 @@ struct _SpiceFileTransferTask
> > > > GFileInputStream *file_stream;
> > > > GFileCopyFlags flags;
> > > > GCancellable *cancellable;
> > > > - GFileProgressCallback progress_callback;
> > > > - gpointer progress_callback_data;
> > > > SpiceFileTransferTaskFlushCb flush_callback;
> > > > gpointer flush_callback_data;
> > > > GAsyncReadyCallback callback;
> > > > @@ -156,6 +152,15 @@ typedef struct {
> > > > SpiceDisplayState display_state;
> > > > } SpiceDisplayConfig;
> > > >
> > > > +typedef struct {
> > > > + GList *tasks;
> > > > + SpiceMainChannel *channel;
> > > > + GFileProgressCallback progress_callback;
> > > > + gpointer progress_callback_data;
> > > > + goffset total_sent;
> > > > + goffset transfer_size;
> > > > +} FileTransferOperation;
> > > > +
> > > > struct _SpiceMainChannelPrivate {
> > > > enum SpiceMouseMode mouse_mode;
> > > > enum SpiceMouseMode requested_mouse_mode;
> > > > @@ -257,6 +262,13 @@ static void file_xfer_flushed(SpiceMainChannel
> > > > *channel,
> > > > gboolean success);
> > > > 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_send_progress(SpiceMainChannel
> > > > *channel,
> > > > SpiceFileTransferTask *xfer_task);
> > > > +static void file_transfer_operation_end(FileTransferOperation *xfer_op);
> > > > +static void file_transfer_operation_reset_all(SpiceMainChannel *channel);
> > > > +static SpiceFileTransferTask
> > > > *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> > > > + gui
> > > > nt32
> > > > task_id);
> > > > +static void file_transfer_operation_task_finished(SpiceMainChannel
> > > > *channel,
> > > > SpiceFileTransferTask *task);
> > > > +
> > > > /* ------------------------------------------------------------------ */
> > > >
> > > > static const char *agent_msg_types[] = {
> > > > @@ -315,8 +327,7 @@ 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->file_xfer_tasks = g_hash_table_new(g_direct_hash, g_direct_equal);
> > > > c->flushing = g_hash_table_new_full(g_direct_hash, g_direct_equal,
> > > > NULL,
> > > > g_object_unref);
> > > > c->cancellable_volume_info = g_cancellable_new();
> > > > @@ -470,9 +481,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;
> > > > @@ -481,15 +489,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);
> > > > }
> > > >
> > > > @@ -1878,7 +1878,9 @@ static void file_xfer_data_flushed_cb(GObject
> > > > *source_object,
> > > > SpiceMainChannel *channel = (SpiceMainChannel *)source_object;
> > > > GError *error = NULL;
> > > >
> > > > - file_xfer_flush_finish(channel, res, &error);
> > > > + if (file_xfer_flush_finish(channel, res, &error))
> > > > + file_transfer_operation_send_progress(channel, self);
> > > > +
> > > Slight change of behavior here. Perviously it seems that we sent the
> > > progress
> > > even if flush_finish was not successful. I don't know what the right
> > > behavior
> > > is. I assume this was intentional?
> > It was but I don't recall exactly the issue that I had. Overall, I think
> > it makes sense to send progress only in case some progress were made
> > instead of failures.
> >
> > >
> > >
> > > >
> > > > spice_file_transfer_task_flush_done(self, error);
> > > > }
> > > >
> > > > @@ -1905,7 +1907,9 @@ static void
> > > > file_xfer_flush_callback(SpiceFileTransferTask *xfer_task,
> > > > {
> > > > SpiceMainChannel *main_channel;
> > > > GCancellable *cancellable;
> > > > + FileTransferOperation *xfer_op = user_data;
> > > >
> > > > + xfer_op->total_sent += count;
> > > > file_xfer_queue(xfer_task, buffer, count);
> > > > if (count == 0)
> > > > return;
> > > > @@ -2146,7 +2150,7 @@ static void main_agent_handle_msg(SpiceChannel
> > > > *channel,
> > > > SpiceFileTransferTask *task;
> > > > VDAgentFileXferStatusMessage *msg = payload;
> > > >
> > > > - task = g_hash_table_lookup(c->file_xfer_tasks,
> > > > GUINT_TO_POINTER(msg-
> > > > >
> > > > > id));
> > > > + task = file_transfer_operation_find_task_by_id(self, msg->id);
> > > > if (task != NULL) {
> > > > spice_file_transfer_task_handle_status(task, msg);
> > > > } else {
> > > > @@ -3042,21 +3046,22 @@ static void
> > > > file_xfer_on_file_info(SpiceFileTransferTask *xfer_task,
> > > > GFileInfo *info,
> > > > gpointer data)
> > > > {
> > > > - SpiceMainChannel *channel;
> > > > GKeyFile *keyfile;
> > > > VDAgentFileXferStartMessage msg;
> > > > gchar *string, *basename;
> > > > guint64 file_size;
> > > > gsize data_len;
> > > > GError *error = NULL;
> > > > + FileTransferOperation *xfer_op;
> > > >
> > > > g_return_if_fail(info != NULL);
> > > > -
> > > > - channel = SPICE_MAIN_CHANNEL(data);
> > > > + xfer_op = data;
> > > >
> > > > basename = g_file_info_get_attribute_as_string(info,
> > > > G_FILE_ATTRIBUTE_STANDARD_NAME);
> > > > file_size = g_file_info_get_attribute_uint64(info,
> > > > G_FILE_ATTRIBUTE_STANDARD_SIZE);
> > > >
> > > > + xfer_op->transfer_size += file_size;
> > > > +
> > > > keyfile = g_key_file_new();
> > > > g_key_file_set_string(keyfile, "vdagent-file-xfer", "name",
> > > > basename);
> > > > g_key_file_set_uint64(keyfile, "vdagent-file-xfer", "size",
> > > > file_size);
> > > > @@ -3072,34 +3077,122 @@ static void
> > > > file_xfer_on_file_info(SpiceFileTransferTask *xfer_task,
> > > >
> > > > /* Create file-xfer start message */
> > > > msg.id = spice_file_transfer_task_get_id(xfer_task);
> > > > - agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_START,
> > > > + agent_msg_queue_many(xfer_op->channel, VD_AGENT_FILE_XFER_START,
> > > > &msg, sizeof(msg),
> > > > string, data_len + 1, NULL);
> > > > g_free(string);
> > > > - spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE);
> > > > + spice_channel_wakeup(SPICE_CHANNEL(xfer_op->channel), FALSE);
> > > > return;
> > > >
> > > > failed:
> > > > spice_file_transfer_task_completed(xfer_task, error);
> > > > }
> > > >
> > > > +static void file_transfer_operation_send_progress(SpiceMainChannel
> > > > *channel,
> > > > SpiceFileTransferTask *xfer_task)
> > > > +{
> > > > + FileTransferOperation *xfer_op;
> > > > + guint32 task_id;
> > > > +
> > > > + 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);
> > > > +
> > > > + if (xfer_op->progress_callback)
> > > > + xfer_op->progress_callback(xfer_op->total_sent,
> > > > + xfer_op->transfer_size,
> > > > + xfer_op->progress_callback_data);
> > > > +}
> > > > +
> > > > +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" */
> > > > + if (xfer_op->tasks != NULL)
> > > > + g_list_free(xfer_op->tasks);
> > > Is there any situation where we would expect tasks to be non-NULL?
> > > Should we at least print a warning for this case?
> > Agreed as it would leak the list pointer, I'll change it.
> >
> > >
> > >
> > > >
> > > > +
> > > > + g_free(xfer_op);
> > > > +}
> > > > +
> > > > +static void file_transfer_operation_reset_all(SpiceMainChannel *channel)
> > > > +{
> > > > + GList *list_op, *it_op;
> > > > +
> > > > + list_op = g_hash_table_get_values(channel->priv->file_xfer_tasks);
> > > > + for (it_op = list_op; it_op != NULL; it_op = it_op->next) {
> > > > + FileTransferOperation *op = it_op->data;
> > > > + GList *it;
> > > > + for (it = op->tasks; it != NULL; it = it->next) {
> > > > + SpiceFileTransferTask *xfer_task =
> > > > SPICE_FILE_TRANSFER_TASK(it-
> > > > >
> > > > > data);
> > > > + GError *error = g_error_new(SPICE_CLIENT_ERROR,
> > > > SPICE_CLIENT_ERROR_FAILED,
> > > > + "Agent connection closed");
> > > > + spice_file_transfer_task_completed(xfer_task, error);
> > > > + }
> > > Since the operation is present in the hash table once for each task that it
> > > contains, it seems that above you will be completing the same task multiple
> > > times for these multi-task operations.
> > >
> > > For example, if Operation A has 4 tasks, g_hash_table_get_values() will
> > > return a
> > > list of 4 items, all containing operation A. So I think you'll call
> > > spice_file_transfer_task_completed() for each of the tasks in operation A
> > > four
> > > times.
> > That's true.. I got myself confused a few times already here due the
> > list_op provided by g_hash_table_get_values on file_xfer_tasks. I think
> > we can improve that based in the comment below.
> >
> > >
> > >
> > > >
> > > > + }
> > > > + g_list_free(list_op);
> > > > +}
> > > > +
> > > > +static SpiceFileTransferTask
> > > > *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> > > > + gui
> > > > nt32
> > > > task_id)
> > > > +{
> > > > + FileTransferOperation *op;
> > > > + GList *it;
> > > > +
> > > > + op = g_hash_table_lookup (channel->priv->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id));
> > > > + g_return_val_if_fail (op != NULL, NULL);
> > > > +
> > > > + for (it = op->tasks; it != NULL; it = it->next) {
> > > > + SpiceFileTransferTask *xfer_task = SPICE_FILE_TRANSFER_TASK(it-
> > > > >
> > > > > data);
> > > > + guint32 id = spice_file_transfer_task_get_id(xfer_task);
> > > > + if (id == task_id)
> > > > + return xfer_task;
> > > > + }
> > > > + return NULL;
> > > > +}
> > > This feels a bit convoluted to me. There is a hash table that relates
> > > a task ID to a FileTransferOperation. The FileTransferOperation has a
> > > list of tasks, and each task has an ID. And since the Operations are
> > > inserted into the hash table once for each task they contain, you may
> > > iterate over the same operation multiple times before you get to the
> > > operation that contains your task. For example, consider where you
> > > have two operations: OperationA with 10 tasks, Operation B with only
> > > 1. And say you're looking for the task in operation B. You could end
> > > up iterating through the ten tasks of operation A ten times (100 total
> > > iterations) before you get to the task you want.
> > Unless I'm missing something, I don't think this is correct. On the code
> > above, we get the correct FileTransferOperation on g_hash_table_lookup
> > (so, in your example, that would be operation B) and the for below would
> > interact only once to find the correct SpiceFileTransferTask;
>
> Yes, you're correct. I clearly read through this code too quickly.
>
> >
> > If the example was about how much we interact in a big
> > FileTransferOperation, for instance, with 100 items, that would be bad
> > indeed and in this case we might consider moving away from GList to
> > GHashTable in the FileTransferOperation. If we agree with this, I would
> > change the _create_tasks() function to return a GHashTable instead of
> > GList.
> >
> > >
> > > Granted, this isn't a performance-critical path, but considering the
> > > other issues mentioned above, I feel there's probably a better design.
> > Indeed, it could be better, what do you think about the following:
> >
> > * channel->priv->file_xfer_tasks: (GHashTable) that provides mapping
> > from task_id to FileTransferOperation ~ useful to get the correct
> > xfer_op based on task_id which is what we receive from Agent
> >
> > * xfer_op->tasks: (GHashTable) that provides mapping from task_id to the
> > actual FileTransferTask pointer ~ This should makes us not worry about
> > performance in large tasks sets.
> >
> > >
> > >
> > > >
> > > > +
> > > > +static void file_transfer_operation_task_finished(SpiceMainChannel
> > > > *channel,
> > > > SpiceFileTransferTask *task)
> > > > +{
> > > > + FileTransferOperation *xfer_op;
> > > > + guint32 task_id;
> > > > +
> > > > + task_id = spice_file_transfer_task_get_id(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(task);
> > > > + return;
> > > > + }
> > > > + /* Remove and free SpiceFileTransferTask */
> > > > + xfer_op->tasks = g_list_remove(xfer_op->tasks, task);
> > > > + g_object_unref(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 (xfer_op->tasks == NULL)
> > > > + file_transfer_operation_end(xfer_op);
> > > > +}
> > > > +
> > > > static void task_finished(SpiceFileTransferTask *task,
> > > > GError *error,
> > > > gpointer data)
> > > > {
> > > > SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
> > > > - guint32 task_id = spice_file_transfer_task_get_id(task);
> > > >
> > > > if (error) {
> > > > VDAgentFileXferStatusMessage msg;
> > > > - msg.id = task_id;
> > > > + msg.id = spice_file_transfer_task_get_id(task);
> > > > msg.result = error->code == G_IO_ERROR_CANCELLED ?
> > > > VD_AGENT_FILE_XFER_STATUS_CANCELLED :
> > > > VD_AGENT_FILE_XFER_STATUS_ERROR;
> > > > agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_STATUS,
> > > > &msg, sizeof(msg), NULL);
> > > > }
> > > >
> > > > - g_hash_table_remove(channel->priv->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id));
> > > > + file_transfer_operation_task_finished(channel, task);
> > > > }
> > > >
> > > > /**
> > > > @@ -3145,7 +3238,8 @@ void spice_main_file_copy_async(SpiceMainChannel
> > > > *channel,
> > > > gpointer user_data)
> > > > {
> > > > SpiceMainChannelPrivate *c = channel->priv;
> > > > - GList *tasks, *it;
> > > > + GList *it;
> > > > + FileTransferOperation *xfer_op;
> > > >
> > > > g_return_if_fail(channel != NULL);
> > > > g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> > > > @@ -3162,27 +3256,29 @@ void spice_main_file_copy_async(SpiceMainChannel
> > > > *channel,
> > > > return;
> > > > }
> > > >
> > > > - tasks = spice_file_transfer_task_create_tasks(channel,
> > > > - sources,
> > > > - flags,
> > > > - cancellable,
> > > > - progress_callback,
> > > > - progress_callback_data,
> > > > - file_xfer_flush_callbac
> > > > k,
> > > > - NULL,
> > > > - callback,
> > > > - user_data);
> > > > - for (it = tasks; it != NULL; it = it->next) {
> > > > + xfer_op = g_new0(FileTransferOperation, 1);
> > > > + xfer_op->progress_callback = progress_callback;
> > > > + xfer_op->progress_callback_data = progress_callback_data;
> > > > + xfer_op->channel = channel;
> > > > + xfer_op->tasks = spice_file_transfer_task_create_tasks(channel,
> > > > + sources,
> > > > + flags,
> > > > + cancellable,
> > > > + file_xfer_flus
> > > > h_ca
> > > > llback,
> > > > + xfer_op,
> > > > + callback,
> > > > + user_data);
> > > What about changing
> > >
> > > GList *spice_file_transfer_task_create_tasks()
> > >
> > > to
> > >
> > > FileTransferOperation spice_file_transfer_operation_create()?
> > I would agree to change the return value to GHashTable but not to
> > FileTransferOperation structure as this is the wrapper from
> > spice_main_file_copy_async data so it makes sense to me to keep this
> > struct internal to channel-main.
>
> Well, perhaps this function itself could be local to channel-main since it's
> basically just wrapping spice_file_transfer_task_new(). On the other hand, I
> don't necessarily think that the FileTransferOperation needs to be internal to
> channel-main. But I get your point.
>
>
> >
> > I don't mind about changing the function name but maybe
> > spice_file_transfer_task_operation_create() instead or just change to
> > singular the previous one, spice_file_transfer_task_create_task as I
> > often associate plural to lists but not to hashtables.
> >
> > >
> > >
> > >
> > > >
> > > > + spice_debug("New file-transfer-operation %p", xfer_op);
> > > > + for (it = xfer_op->tasks; it != NULL; it = it->next) {
> > > > SpiceFileTransferTask *task = SPICE_FILE_TRANSFER_TASK(it->data);
> > > > guint32 task_id = spice_file_transfer_task_get_id(task);
> > > >
> > > > - g_hash_table_insert(c->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id),
> > > > task);
> > > > - g_signal_connect(task, "file-info",
> > > > G_CALLBACK(file_xfer_on_file_info), channel);
> > > > + g_hash_table_insert(c->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id),
> > > > xfer_op);
> > > > + g_signal_connect(task, "file-info",
> > > > G_CALLBACK(file_xfer_on_file_info), xfer_op);
> > > > g_signal_connect(task, "finished", G_CALLBACK(task_finished),
> > > > channel);
> > > > g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> > > > task);
> > > > spice_file_transfer_task_start_task(task);
> > > > }
> > > > - g_list_free(tasks);
> > > > }
> > > >
> > > > /**
> > > > @@ -3251,26 +3347,6 @@ static void
> > > > spice_file_transfer_task_flush_done(SpiceFileTransferTask *self, GEr
> > > > }
> > > > }
> > > >
> > > > - if (self->progress_callback) {
> > > > - goffset read = 0;
> > > > - goffset total = 0;
> > > > - SpiceMainChannel *main_channel = self->channel;
> > > > - GHashTableIter iter;
> > > > - gpointer key, value;
> > > > -
> > > > - /* 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, main_channel->priv-
> > > > >file_xfer_tasks);
> > > > - while (g_hash_table_iter_next(&iter, &key, &value)) {
> > > > - SpiceFileTransferTask *t = (SpiceFileTransferTask *)value;
> > > > - read += t->read_bytes;
> > > > - total += t->file_size;
> > > > - }
> > > > -
> > > > - self->progress_callback(read, total, self-
> > > > >progress_callback_data);
> > > > - }
> > > > -
> > > > /* Read more data */
> > > > file_xfer_continue_read(self);
> > > > }
> > > > @@ -3279,8 +3355,6 @@ static GList
> > > > *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> > > > GFile **files,
> > > > GFileCopyFlags flags,
> > > > GCancellable
> > > > *cancellable,
> > > > - GFileProgressCallback
> > > > progress_callback,
> > > > - gpointer
> > > > progress_callback_data,
> > > > SpiceFileTransferTask
> > > > Flus
> > > > hCb flush_callback,
> > > > gpointer
> > > > flush_callback_data,
> > > > GAsyncReadyCallback
> > > > callback,
> > > > @@ -3300,8 +3374,6 @@ static GList
> > > > *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> > > >
> > > > task = spice_file_transfer_task_new(channel, files[i],
> > > > task_cancellable);
> > > > task->flags = flags;
> > > > - task->progress_callback = progress_callback;
> > > > - task->progress_callback_data = progress_callback_data;
> > > > task->flush_callback = flush_callback;
> > > > task->flush_callback_data = flush_callback_data;
> > > > task->callback = callback;
> > >
> > > Perhaps it would be simpler to simply keep the hash table as a map between
> > > task
> > > ID and SpiceFileTransferTask, but add a field to SpiceFileTransferTask
> > > pointing
> > > to its operation?
> > That would do as well! (sorry, I should have read the whole email before
> > starting to reply).
>
> To be fair, I didn't write my review in the best order either. New bits of code
> triggered new ideas, so my thinking changed slightly over the course of the
> review ;)
>
> >
> > So, with my proposal, we would do:
> > 1a) from task_id gets FileTransferOperation (xfer_op) from
> > channel->priv->file_xfer_tasks
> > 2a) from task_id gets SpiceFileTransferTask (xfer_task) from xfer_op's
> > GHashTable
> >
> > With your proposal, we would do:
> > 1b) from task_id gets SpiceFileTransferTask (xfer_task) from
> > channel->priv->file_xfer_tasks
> > 2b) from xfer_task gets xfer_op
> >
> > I still prefer my proposal because we wouldn't need to share
> > FileTransferOperation logic with SpiceFileTransferTask.
>
> It's true that my suggestion does allow the FileTransferOperation to bleed over
> into SpiceFileTransferTask. On the other hand, it does make a few things much
> simpler and more straightforward (as mentioned above). So I think I would
> personally prefer to expose the FileTansferOperation to SpiceFileTransferTask if
> it makes the rest of the implementation cleaner. But that's mostly a personal
> preference.
As there is a lot of things going on, I'll do the next version with my
proposal (mostly because it is easier for me) and we can re-evaluate
then if we should change.
Cheers,
toso
>
> >
> > >
> > >
> > > Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > Let me know what you think!
> > 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