[Spice-devel] [spice-gtk v3 07/16] file-xfer: call user callback once per operation
Jonathon Jongsma
jjongsma at redhat.com
Tue Jun 7 16:47:12 UTC 2016
On Sun, 2016-06-05 at 11:39 +0200, Victor Toso wrote:
> Hi,
>
> On Fri, Jun 03, 2016 at 12:12:14PM -0500, Jonathon Jongsma wrote:
> >
> > On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> > >
> > > SpiceFileTransferTask has a callback to be called when operation
> > > ended. Til this patch, we were setting the user callback which means
> > > that in multiple file-transfers, we were calling the user callback
> > > several times.
> > >
> > > Following the same logic pointed from 113093dd00a1cf10f6d3c3589b7 this
> > > is a SpiceMainChannel operation and it should only call the user
> > > callback when this operation is over (FileTransferOperation now).
> > > ---
> > > src/channel-main.c | 72 ++-
> > > src/tmp-introspect325cwcm0/SpiceClientGtk-3.0.c | 628
> > > ++++++++++++++++++++++++
> > > 2 files changed, 694 insertions(+), 6 deletions(-)
> > > create mode 100644 src/tmp-introspect325cwcm0/SpiceClientGtk-3.0.c
> > >
> > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > index f36326d..e204a1e 100644
> > > --- a/src/channel-main.c
> > > +++ b/src/channel-main.c
> > > @@ -157,6 +157,10 @@ typedef struct {
> > > SpiceMainChannel *channel;
> > > GFileProgressCallback progress_callback;
> > > gpointer progress_callback_data;
> > > + GAsyncReadyCallback end_callback;
> > > + gpointer end_callback_data;
> > > + GError *error;
> > > + GCancellable *cancellable;
> > > goffset total_sent;
> > > goffset transfer_size;
> > > } FileTransferOperation;
> > > @@ -1838,9 +1842,7 @@ static void file_xfer_close_cb(GObject *object,
> > > }
> > > }
> > >
> > > - /* Notify to user that files have been transferred or something error
> > > - happened. */
> > > - task = g_task_new(self->channel,
> > > + task = g_task_new(self,
> > > self->cancellable,
> > > self->callback,
> > > self->user_data);
> > > @@ -1919,6 +1921,42 @@ static void
> > > file_xfer_flush_callback(SpiceFileTransferTask *xfer_task,
> > > file_xfer_flush_async(main_channel, cancellable,
> > > file_xfer_data_flushed_cb, xfer_task);
> > > }
> > >
> > > +static void file_xfer_end_callback(GObject *source_object,
> > > + GAsyncResult *res,
> > > + gpointer user_data)
> > > +{
> > > + GTask *task;
> > > + FileTransferOperation *xfer_op;
> > > +
> > > + task = G_TASK(res);
> > > + if (!g_task_had_error(task))
> > > + /* SpiceFileTransferTask and FileTransferOperation are freed on
> > > + * file_transfer_operation_task_finished */
> > > + return;
> > In general, a GAsyncReadyCallback is expected to call a _finish() function
> > to
> > get the error status. Since this is all internal, we can poke around at the
> > implementation of the asynchronous task to find the error, but I think I'd
> > prefer to provide a _finish() function to follow the conventions.
> >
> Agreed, I'll improve it
>
> >
> > >
> > > +
> > > + xfer_op = user_data;
> > > +
> > > + if (xfer_op->error != NULL)
> > > + return;
> > > +
> > > + /* Get the GError from SpiceFileTransferTask so we can properly
> > > return to
> > > + * the application when the FileTransferOperation ends */
> > > + g_task_propagate_boolean(task, &xfer_op->error);
> > since file_xfer_end_callback() is called once for each file in the
> > operation,
> > the above line will overwrite xfer_op->error each time it is called. So
> > we'll
> > only report the error status of the last file that finished. We probably
> > don't
> > want to overwrite an earlier error with a later success.
> >
> Not really, because the check for xfer_op->error above, right? If we got
> an xfer_op->error before, we will return an error in the operation
> unless it is the cancelation case handled below, which does
> g_clear_error in the end making it null again.
Again, I read too quickly. Sorry for the misinformation.
>
> Nevertheless, can be improved.
>
> Should we somehow include the error of each SpiceFileTransferTask in the
> end? Let's say, if we have 8 operations and 2 failed. Should we include
> in one GError the fact that 2 xfer_task failed? I'm only returning the
> first GError so far (if the logic above is correct)
Yeah, this is an interesting issue. The client currently handles errors via the
"finished" signal for each SpiceFileTransferTask (though we don't really report
errors properly in the client UI yet). And the fact is, I think the only place
that currently uses this spice_main_file_copy_async() function is in spice-
widget.c (drag-and-drop handling), which passes NULL for the
GAsyncReadyCallback. So at the moment, I believe any error we return here is
never handled by anyone. So while I think that it would be good to improve it,
I'm not sure what the right behavior would be, and it's mostly a theoretical
issue at the moment ;)
>
> >
> > >
> > > +
> > > + /* User can cancel a FileTransfer without cancelling the whole
> > > + * operation. For that, spice_main_file_copy_async must be called
> > > + * without GCancellabe */
> > > + if (g_error_matches(xfer_op->error, G_IO_ERROR, G_IO_ERROR_CANCELLED)
> > > &&
> > > + xfer_op->cancellable == NULL) {
> > > + SpiceFileTransferTask *xfer_task;
> > > +
> > > + xfer_task = SPICE_FILE_TRANSFER_TASK(source_object);
> > > + spice_debug("file-transfer %u was cancelled",
> > > + spice_file_transfer_task_get_id(xfer_task));
> > > + g_clear_error(&xfer_op->error);
> > > + }
> > > +}
> > > +
> > > /* main context */
> > > static void file_xfer_read_cb(GObject *source_object,
> > > GAsyncResult *res,
> > > @@ -3108,10 +3146,24 @@ 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);
> > >
> > > + if (xfer_op->end_callback) {
> > > + GTask *task = g_task_new(xfer_op->channel,
> > > + xfer_op->cancellable,
> > > + xfer_op->end_callback,
> > > + xfer_op->end_callback_data);
> > > +
> > > + if (xfer_op->error != NULL) {
> > > + g_task_return_error(task, xfer_op->error);
> > > + } else {
> > > + g_task_return_boolean(task, TRUE);
> > > + }
> > > + }
> > > +
> > If FileTransferOperation owned a GTask* member, we wouldn't have to store
> > these
> > 'cancellable', 'end_callback', and 'end_callback_data' fields separately. I
> > know
> > that it's done elsewhere in the code, but it seems a bit odd to create a
> > GTask
> > right as the task is done and essentially use it only to trigger the
> > callback.
> > Also, the cancellable doesn't do much good if the GTask only lives long
> > enough
> > to be created and return ;)
> >
> Indeed :)
>
> >
> >
> > >
> > > /* SpiceFileTransferTask itself is freed after it emits "finish" */
> > > if (xfer_op->tasks != NULL)
> > > g_list_free(xfer_op->tasks);
> > >
> > > + g_clear_object (&xfer_op->cancellable);
> > > g_free(xfer_op);
> > > }
> > >
> > > @@ -3225,7 +3277,11 @@ static void task_finished(SpiceFileTransferTask
> > > *task,
> > > * files, please connect to the #SpiceMainChannel::new-file-transfer
> > > signal.
> > > *
> > > * When the operation is finished, callback will be called. You can then
> > > call
> > > - * spice_main_file_copy_finish() to get the result of the operation.
> > > + * spice_main_file_copy_finish() to get the result of the operation. Note
> > > that
> > > + * before release 0.32 the callback was called for each file in multiple
> > > file
> > > + * transfer. This behavior was changed for the same reason as the
> > > + * progress_callback (above). If you need to monitor the ending of
> > > individual
> > > + * files, you can connect to "finished" signal from each
> > > SpiceFileTransferTask.
> > > *
> > > **/
> > > void spice_main_file_copy_async(SpiceMainChannel *channel,
> > > @@ -3259,15 +3315,19 @@ void spice_main_file_copy_async(SpiceMainChannel
> > > *channel,
> > > xfer_op = g_new0(FileTransferOperation, 1);
> > > xfer_op->progress_callback = progress_callback;
> > > xfer_op->progress_callback_data = progress_callback_data;
> > > + xfer_op->end_callback = callback;
> > > + xfer_op->end_callback_data = user_data;
> > > xfer_op->channel = channel;
> > > + xfer_op->error = NULL;
> > > + xfer_op->cancellable = (cancellable != NULL) ?
> > > g_object_ref(cancellable)
> > > : NULL;
> > > xfer_op->tasks = spice_file_transfer_task_create_tasks(channel,
> > > sources,
> > > flags,
> > > cancellable,
> > > file_xfer_flus
> > > h_ca
> > > llback,
> > > xfer_op,
> > > - callback,
> > > - user_data);
> > > + file_xfer_end_
> > > call
> > > back,
> > > + xfer_op);
> > > 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);
> >
> > I assume the following file was accidentally committed?
> I have no clue how this file got here, haha.
>
> I'll improve this in v4, thanks!
>
More information about the Spice-devel
mailing list