Hi,
On Mon, Aug 01, 2016 at 12:43:10PM +0200, Christophe Fergeau wrote:
> On Sat, Jul 30, 2016 at 12:26:27AM +0200, Victor Toso wrote:
> > The self->error is the error set for the file-transfer and it will be
> > propagated with the "finish" signal. As this is transfer none pointer,
> > we should clear it out afterwards.
> >
> > 40 (16 direct, 24 indirect) bytes in 1 blocks are definitely lost in
> > loss record 7,489 of 14,298
> > at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> > by 0xB5090E8: g_malloc (gmem.c:94)
> > by 0xB51F8A2: g_slice_alloc (gslice.c:1025)
> > by 0xB4EFCC5: g_error_new_literal (gerror.c:471)
> > by 0xB4EFFAD: g_set_error_literal (gerror.c:619)
> > by 0xAF13397: g_cancellable_set_error_if_cancelled (gcancellable.c:314)
> > by 0xAF630C8: g_task_propagate_error (gtask.c:1519)
> > by 0xAF63CD8: g_task_propagate_int (gtask.c:1652)
> > by 0x50863F5: spice_file_transfer_task_read_finish (spice-file-transfer-task.c:477)
> > by 0x5093239: file_xfer_read_async_cb (channel-main.c:1811)
> > by 0xAF62F38: g_task_return_now (gtask.c:1121)
> > by 0xAF63775: g_task_return (gtask.c:1179)
> > ---
> > src/spice-file-transfer-task.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
> > index ea46c9d..667d230 100644
> > --- a/src/spice-file-transfer-task.c
> > +++ b/src/spice-file-transfer-task.c
> > @@ -589,6 +589,7 @@ spice_file_transfer_task_dispose(GObject *object)
> >
> > g_clear_object(&self->file);
> > g_clear_object(&self->file_stream);
> > + g_clear_error(&self->error);
> >
> > G_OBJECT_CLASS(spice_file_transfer_task_parent_class)->dispose(object);
>
> I think you'll need at least something like
> diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
> index 70571f0..ba99e1f 100644
> --- a/src/spice-file-transfer-task.c
> +++ b/src/spice-file-transfer-task.c
> @@ -122,6 +122,7 @@ static void spice_file_transfer_task_query_info_cb(GObject *obj,
> g_clear_error(&error);
> /* Return error previously reported */
> g_task_return_error(task, self->error);
> + self->error = NULL;
Not really.
SpiceFileTransferTask::error is NULL when file-transfer start and if it
is set, it means that file-transfer has failed. We should keep it set
till the very end unless the error is something we can recover from.
The g_task_return_error() will inform the caller (SpiceMainChannel) that
this xfer-task has failed and in that situation it will call
spice_file_transfer_task_completed(). The GError argument in
_task_completed() is for external errors (e.g. Agent had an error) so,
SpiceMainChannel might not call _task_completed() with the GError set.
If you agree with what I've said, we might improve the documentation to
have this more clear; changing the design/usage of self->error might be
possible too, but I don't think it is necessary.
Thanks for the reviews,
toso
> g_object_unref(task);
> return;
> } else if (error) {
> @@ -160,6 +161,7 @@ static void spice_file_transfer_task_read_file_cb(GObject *obj,
> /* Return error previously reported */
> self->pending = FALSE;
> g_task_return_error(task, self->error);
> + self->error = NULL;
> g_object_unref(task);
> return;
> } else if (error) {
> @@ -198,6 +200,7 @@ static void spice_file_transfer_task_read_stream_cb(GObject *source_object,
> g_clear_error(&error);
> /* On any pending error on SpiceFileTransferTask */
> g_task_return_error(task, self->error);
> + self->error = NULL;
> g_object_unref(task);
> return;
> } else if (error) {
>
>
> (or to g_error_copy() the error you pass to g_task_return_error()).
> Though I must admit I haven't been able to easily follow the codepaths
> handling self->error, so maybe I missed something.
>
> Christophe
> _______________________________________________
> 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