[Spice-commits] 8 commits - src/channel-main.c src/spice-file-transfer-task.c src/spice-file-transfer-task-priv.h src/spicy.c tests/file-transfer.c
Victor Toso de Carvalho
victortoso at kemper.freedesktop.org
Wed Aug 3 13:30:38 UTC 2016
src/channel-main.c | 19 +++++++++++++------
src/spice-file-transfer-task-priv.h | 1 +
src/spice-file-transfer-task.c | 36 ++++++++++++++++++++++++++++++++----
src/spicy.c | 5 ++++-
tests/file-transfer.c | 7 +++++++
5 files changed, 57 insertions(+), 11 deletions(-)
New commits:
commit e1749021674518f54500c417d961576cf31ce51e
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Wed Jul 27 18:15:53 2016 +0200
spicy: Fix spice_file_transfer_task_get_filename leak
It was wrongly annotated as (transfer none)
An alternative would be to store what is returned by
g_file_get_basename() in SpiceFileTransferTask and return that, this
would allow to make spice_file_transfer_task_get_filename() (transfer
none) without leaking memory.
Acked-by: Victor Toso <victortoso at redhat.com>
diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index 219e980..2207ba4 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -535,7 +535,7 @@ void spice_file_transfer_task_cancel(SpiceFileTransferTask *self)
*
* Gets the name of the file being transferred in this task
*
- * Returns: (transfer none): The basename of the file
+ * Returns: (transfer full): The basename of the file
*
* Since: 0.31
**/
diff --git a/src/spicy.c b/src/spicy.c
index ea4d4e0..3cee7e5 100644
--- a/src/spicy.c
+++ b/src/spicy.c
@@ -1514,6 +1514,7 @@ task_cancel_cb(GtkButton *button,
static TransferTaskWidgets *
transfer_task_widgets_new(SpiceFileTransferTask *task)
{
+ char *filename;
TransferTaskWidgets *widgets = g_new0(TransferTaskWidgets, 1);
widgets->vbox = gtk_box_new(GTK_ORIENTATION_VERTICAL, 0);
@@ -1521,7 +1522,9 @@ transfer_task_widgets_new(SpiceFileTransferTask *task)
widgets->cancel = gtk_button_new_with_label("Cancel");
widgets->progress = gtk_progress_bar_new();
- widgets->label = gtk_label_new(spice_file_transfer_task_get_filename(task));
+ filename = spice_file_transfer_task_get_filename(task);
+ widgets->label = gtk_label_new(filename);
+ g_free(filename);
gtk_widget_set_halign(widgets->label, GTK_ALIGN_START);
gtk_widget_set_valign(widgets->label, GTK_ALIGN_BASELINE);
commit acda160767802f8d8ba07d591db3a2a566197472
Author: Victor Toso <victortoso at redhat.com>
Date: Fri Jul 29 17:37:25 2016 +0200
channel-main: avoid race around file-transfer flush
This patch avoids a race condition. The race happens when we mark the
xfer-task as completed by receiving VD_AGENT_FILE_XFER_STATUS_SUCCESS
message while the flush callback was not yet called.
The flush callback is file_xfer_data_flushed_cb() and it might not be
called immediately after data is flushed.
The race can be verified while transferring several small files at
once. I can see it often with more then 50 files in one transfer
operation.
This fix implies that SpiceMainChannel should check in its async
callbacks if given SpiceFileTransferTask is completed.
This patch introduces spice_file_transfer_task_is_completed (internal)
to help check if spice_file_transfer_task_completed() was called or
not.
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/src/channel-main.c b/src/channel-main.c
index 5af73b4..d9fd025 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1766,10 +1766,12 @@ static void file_xfer_data_flushed_cb(GObject *source_object,
return;
}
- file_transfer_operation_send_progress(xfer_task);
-
- /* Read more data */
- spice_file_transfer_task_read_async(xfer_task, file_xfer_read_async_cb, NULL);
+ /* task might be completed while on idle */
+ 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);
+ }
}
static void file_xfer_queue_msg_to_agent(SpiceMainChannel *channel,
@@ -1814,13 +1816,15 @@ static void file_xfer_read_async_cb(GObject *source_object,
}
file_xfer_queue_msg_to_agent(channel, spice_file_transfer_task_get_id(xfer_task), buffer, count);
- if (count == 0) {
- /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent */
+ if (count == 0 || spice_file_transfer_task_is_completed(xfer_task)) {
+ /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent
+ * in case the task was completed, nothing to do. */
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);
@@ -1841,6 +1845,7 @@ static void main_agent_handle_xfer_status(SpiceMainChannel *channel,
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);
return;
case VD_AGENT_FILE_XFER_STATUS_CANCELLED:
diff --git a/src/spice-file-transfer-task-priv.h b/src/spice-file-transfer-task-priv.h
index 1ceb392..a7b58d8 100644
--- a/src/spice-file-transfer-task-priv.h
+++ b/src/spice-file-transfer-task-priv.h
@@ -52,6 +52,7 @@ gssize spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
GError **error);
guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask *self);
guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask *self);
+gboolean spice_file_transfer_task_is_completed(SpiceFileTransferTask *self);
G_END_DECLS
diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index a2b3885..219e980 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -42,6 +42,7 @@ struct _SpiceFileTransferTask
GObject parent;
uint32_t id;
+ gboolean completed;
gboolean pending;
GFile *file;
SpiceMainChannel *channel;
@@ -270,6 +271,8 @@ G_GNUC_INTERNAL
void spice_file_transfer_task_completed(SpiceFileTransferTask *self,
GError *error)
{
+ self->completed = TRUE;
+
/* In case of multiple errors we only report the first error */
if (self->error)
g_clear_error(&error);
@@ -477,6 +480,16 @@ gssize spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
return nbytes;
}
+G_GNUC_INTERNAL
+gboolean spice_file_transfer_task_is_completed(SpiceFileTransferTask *self)
+{
+ g_return_val_if_fail(self != NULL, FALSE);
+
+ /* File transfer is considered over at the first time it is marked as
+ * complete with spice_file_transfer_task_completed. */
+ return self->completed;
+}
+
/*******************************************************************************
* External API
******************************************************************************/
commit 9c5349934296f41c1d5110fe5b3db8dffd248931
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Wed Jul 27 15:48:29 2016 +0200
file-transfer: avoid potential leaks
Acked-by: Victor Toso <victortoso at redhat.com>
diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index 5bb6359..a2b3885 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -116,6 +116,8 @@ static void spice_file_transfer_task_query_info_cb(GObject *obj,
info = g_file_query_info_finish(G_FILE(obj), res, &error);
if (self->error) {
+ g_clear_object(&info);
+ g_clear_error(&error);
/* Return error previously reported */
g_task_return_error(task, g_error_copy(self->error));
g_object_unref(task);
@@ -152,6 +154,7 @@ static void spice_file_transfer_task_read_file_cb(GObject *obj,
self->file_stream = g_file_read_finish(G_FILE(obj), res, &error);
if (self->error) {
+ g_clear_error(&error);
/* Return error previously reported */
self->pending = FALSE;
g_task_return_error(task, g_error_copy(self->error));
@@ -190,6 +193,7 @@ static void spice_file_transfer_task_read_stream_cb(GObject *source_object,
nbytes = g_input_stream_read_finish(G_INPUT_STREAM(self->file_stream), res, &error);
if (self->error) {
+ g_clear_error(&error);
/* On any pending error on SpiceFileTransferTask */
g_task_return_error(task, g_error_copy(self->error));
g_object_unref(task);
commit 0344cfbdc5f03ac3aeb7ea1b87cfa3e4e9f051e7
Author: Victor Toso <victortoso at redhat.com>
Date: Fri Jul 29 23:02:32 2016 +0200
file-transfer: Fix SpiceFileTransferTask::error leak
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 not lose its reference on g_task_return_error and 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)
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index ea46c9d..5bb6359 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -117,7 +117,7 @@ static void spice_file_transfer_task_query_info_cb(GObject *obj,
info = g_file_query_info_finish(G_FILE(obj), res, &error);
if (self->error) {
/* Return error previously reported */
- g_task_return_error(task, self->error);
+ g_task_return_error(task, g_error_copy(self->error));
g_object_unref(task);
return;
} else if (error) {
@@ -154,7 +154,7 @@ static void spice_file_transfer_task_read_file_cb(GObject *obj,
if (self->error) {
/* Return error previously reported */
self->pending = FALSE;
- g_task_return_error(task, self->error);
+ g_task_return_error(task, g_error_copy(self->error));
g_object_unref(task);
return;
} else if (error) {
@@ -191,7 +191,7 @@ static void spice_file_transfer_task_read_stream_cb(GObject *source_object,
nbytes = g_input_stream_read_finish(G_INPUT_STREAM(self->file_stream), res, &error);
if (self->error) {
/* On any pending error on SpiceFileTransferTask */
- g_task_return_error(task, self->error);
+ g_task_return_error(task, g_error_copy(self->error));
g_object_unref(task);
return;
} else if (error) {
@@ -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);
}
commit a793a956f4f41e5bccb263a133e2e0f4e042cd3f
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Wed Jul 27 15:30:21 2016 +0200
file-transfer: Fix SpiceFileTransferTask::file_stream leak
g_file_read_finish() is (transfer full) so we must release the ref
we got in _dispose() as it's not done anywhere else in the code.
Acked-by: Victor Toso <victortoso at redhat.com>
diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index 90c2a5d..ea46c9d 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -588,6 +588,7 @@ spice_file_transfer_task_dispose(GObject *object)
SpiceFileTransferTask *self = SPICE_FILE_TRANSFER_TASK(object);
g_clear_object(&self->file);
+ g_clear_object(&self->file_stream);
G_OBJECT_CLASS(spice_file_transfer_task_parent_class)->dispose(object);
}
commit 55c70ac51f7268f82683be6eef790ef17a1d8e5d
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Wed Jul 27 15:28:32 2016 +0200
file-transfer: Fix GTask leaks
The file transfer code calls g_*_async() methods with a GTask as the
user_data argument. It needs to own a reference to the GTask for the
duration of the async call to keep the GTask alive, however it was never
releasing it when it no longer needs it (that is, after calling
g_task_return_*).
Some of the leaks fixed by this patch are:
* GTask leak after flush
11,232 bytes in 54 blocks are definitely lost in
loss record 14,018 of 14,071
at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
by 0xC14A0E8: g_malloc (gmem.c:94)
by 0xC1608A2: g_slice_alloc (gslice.c:1025)
by 0xC160ECD: g_slice_alloc0 (gslice.c:1051)
by 0xBEDBD11: g_type_create_instance (gtype.c:1857)
by 0xBEBD7DA: g_object_new_internal (gobject.c:1781)
by 0xBEBF11C: g_object_newv (gobject.c:1928)
by 0xBEBF89B: g_object_new (gobject.c:1621)
by 0xBBA4424: g_task_new (gtask.c:693)
by 0x6F8C3C0: file_xfer_flush_async (channel-main.c:899)
by 0x6F8C3C0: file_xfer_read_async_cb (channel-main.c:1826)
by 0xBBA3F38: g_task_return_now (gtask.c:1121)
by 0xBBA4775: g_task_return (gtask.c:1179)
* GTask leak from spice_main_file_copy_async()
208 bytes in 1 blocks are definitely lost in
loss record 13,708 of 15,093
at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
by 0xC14A0E8: g_malloc (gmem.c:94)
by 0xC1608A2: g_slice_alloc (gslice.c:1025)
by 0xC160ECD: g_slice_alloc0 (gslice.c:1051)
by 0xBEDBD11: g_type_create_instance (gtype.c:1857)
by 0xBEBD7DA: g_object_new_internal (gobject.c:1781)
by 0xBEBF11C: g_object_newv (gobject.c:1928)
by 0xBEBF89B: g_object_new (gobject.c:1621)
by 0xBBA4424: g_task_new (gtask.c:693)
by 0x6F8EF55: spice_main_file_copy_async (channel-main.c:3084)
Signed-off-by: Victor Toso <victortoso at redhat.com>
Acked-by: Victor Toso <victortoso at redhat.com>
diff --git a/src/channel-main.c b/src/channel-main.c
index 26192ed..5af73b4 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -936,6 +936,7 @@ static void agent_send_msg_queue(SpiceMainChannel *channel)
if (task) {
/* if there's a flush task waiting for this message, finish it */
g_task_return_boolean(task, TRUE);
+ g_object_unref(task);
g_hash_table_remove(c->flushing, out);
}
}
@@ -2887,6 +2888,7 @@ static void file_transfer_operation_free(FileTransferOperation *xfer_op)
SPICE_DEBUG("Transfer successful (%p)", xfer_op);
g_task_return_boolean(xfer_op->task, TRUE);
}
+ g_object_unref(xfer_op->task);
/* SpiceFileTransferTask itself is freed after it emits "finish" */
g_hash_table_unref(xfer_op->xfer_task);
diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index 23aab62..90c2a5d 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -118,9 +118,11 @@ static void spice_file_transfer_task_query_info_cb(GObject *obj,
if (self->error) {
/* Return error previously reported */
g_task_return_error(task, self->error);
+ g_object_unref(task);
return;
} else if (error) {
g_task_return_error(task, error);
+ g_object_unref(task);
return;
}
@@ -132,6 +134,7 @@ static void spice_file_transfer_task_query_info_cb(GObject *obj,
g_object_notify(G_OBJECT(self), "progress");
g_task_return_pointer(task, info, g_object_unref);
+ g_object_unref(task);
}
static void spice_file_transfer_task_read_file_cb(GObject *obj,
@@ -152,10 +155,12 @@ 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);
+ g_object_unref(task);
return;
} else if (error) {
self->pending = FALSE;
g_task_return_error(task, error);
+ g_object_unref(task);
return;
}
@@ -187,9 +192,11 @@ static void spice_file_transfer_task_read_stream_cb(GObject *source_object,
if (self->error) {
/* On any pending error on SpiceFileTransferTask */
g_task_return_error(task, self->error);
+ g_object_unref(task);
return;
} else if (error) {
g_task_return_error(task, error);
+ g_object_unref(task);
return;
}
@@ -209,6 +216,7 @@ static void spice_file_transfer_task_read_stream_cb(GObject *source_object,
}
g_task_return_int(task, nbytes);
+ g_object_unref(task);
}
/* main context */
@@ -433,6 +441,7 @@ void spice_file_transfer_task_read_async(SpiceFileTransferTask *self,
* reach a state where agent says file-transfer SUCCEED but we are in a
* PENDING state in SpiceFileTransferTask due reading in idle */
g_task_return_int(task, 0);
+ g_object_unref(task);
return;
}
commit 377842b94d15929788ae24203d1a37290bee7462
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Wed Jul 27 15:27:30 2016 +0200
test-file-transfer: Don't leak GError
Acked-by: Victor Toso <victortoso at redhat.com>
diff --git a/tests/file-transfer.c b/tests/file-transfer.c
index 4e00e9a..a27b9cf 100644
--- a/tests/file-transfer.c
+++ b/tests/file-transfer.c
@@ -152,6 +152,7 @@ transfer_cancelled_on_init_async_cb(GObject *obj, GAsyncResult *res, gpointer da
info = spice_file_transfer_task_init_task_finish(xfer_task, res, &error);
g_assert_error(error, G_IO_ERROR, G_IO_ERROR_CANCELLED);
g_assert_null(info);
+ g_clear_error(&error);
transfer_xfer_task_on_finished(NULL, NULL, data);
}
@@ -208,6 +209,7 @@ transfer_cancelled_read_async_cb(GObject *source_object,
count = spice_file_transfer_task_read_finish(xfer_task, res, &buffer, &error);
g_assert_error(error, G_IO_ERROR, G_IO_ERROR_CANCELLED);
g_assert_cmpint(count, ==, -1);
+ g_clear_error(&error);
transfer_xfer_task_on_finished(NULL, NULL, user_data);
}
@@ -300,6 +302,7 @@ transfer_agent_cancelled_read_async_cb(GObject *source_object,
count = spice_file_transfer_task_read_finish(xfer_task, res, &buffer, &error);
g_assert_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED);
g_assert_cmpint(count, ==, -1);
+ g_clear_error(&error);
transfer_xfer_task_on_finished(NULL, NULL, user_data);
}
commit 66d8e2ee5070a1f1a629f9948d046dea21fb3601
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Wed Jul 27 12:51:56 2016 +0200
test-file-transfer: Don't leak GFileInfo
The GFileInfo returned by spice_file_transfer_task_init_task_finish()
must be unref'ed when no longer needed.
Acked-by: Victor Toso <victortoso at redhat.com>
diff --git a/tests/file-transfer.c b/tests/file-transfer.c
index d4d216d..4e00e9a 100644
--- a/tests/file-transfer.c
+++ b/tests/file-transfer.c
@@ -116,6 +116,7 @@ transfer_init_async_cb(GObject *obj, GAsyncResult *res, gpointer data)
info = spice_file_transfer_task_init_task_finish(xfer_task, res, &error);
g_assert_no_error(error);
g_assert_nonnull(info);
+ g_object_unref(info);
/* read file loop */
spice_file_transfer_task_read_async(xfer_task, transfer_read_async_cb, NULL);
@@ -223,6 +224,7 @@ transfer_on_init_async_cb_before_read_cancel(GObject *obj, GAsyncResult *res, gp
info = spice_file_transfer_task_init_task_finish(xfer_task, res, &error);
g_assert_no_error(error);
g_assert_nonnull(info);
+ g_object_unref(info);
cancellable = spice_file_transfer_task_get_cancellable(xfer_task);
g_cancellable_cancel(cancellable);
@@ -241,6 +243,7 @@ transfer_on_init_async_cb_after_read_cancel(GObject *obj, GAsyncResult *res, gpo
info = spice_file_transfer_task_init_task_finish(xfer_task, res, &error);
g_assert_no_error(error);
g_assert_nonnull(info);
+ g_object_unref(info);
cancellable = spice_file_transfer_task_get_cancellable(xfer_task);
spice_file_transfer_task_read_async(xfer_task, transfer_cancelled_read_async_cb, data);
@@ -313,6 +316,7 @@ transfer_on_init_async_cb_agent_cancel(GObject *obj, GAsyncResult *res, gpointer
info = spice_file_transfer_task_init_task_finish(xfer_task, res, &error);
g_assert_no_error(error);
g_assert_nonnull(info);
+ g_object_unref(info);
spice_file_transfer_task_read_async(xfer_task, transfer_agent_cancelled_read_async_cb, data);
More information about the Spice-commits
mailing list