Hey,
On Tue, Aug 02, 2016 at 11:48:48AM +0200, Victor Toso wrote:
> channel-main uses the SpiceFileTransferTask reference given in the
> hash table provided by spice_file_transfer_task_create_tasks(). That
> means SpiceFileTransferTask is not holding a reference for itself but
> it relies on it due the async calls it provides.
>
> This patch increases the reference of each SpiceFileTransferTask for
> the hash table which will be unref'ed by channel-main in
> file_transfer_operation_task_finished(); the original reference is
> kept to SpiceFileTransferTask to be freed after the finish signal is
> emitted on spice_file_transfer_task_close_stream_cb().
>
> We are changing the GHashTable creation using g_hash_table_full() with
> g_object_unref() to simplify the usage of this GHashTable.
I believe this bit could be split up, first patch would become
- g_hash_table_insert(xfer_ht, GUINT_TO_POINTER(task_id), xfer_task);
+ g_hash_table_insert(xfer_ht, GUINT_TO_POINTER(task_id), g_object_ref(xfer_task));
+ added comments maybe, and second patch would change to use
g_hash_table_new_full and simplify memory handling of the hash table
elements.
>
> This patch fixes some critical warnings as we have two g_object_unref
> but only one reference.
> ---
> src/channel-main.c | 4 ----
> src/spice-file-transfer-task.c | 11 ++++++-----
> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 5af73b4..bc7349f 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -2889,8 +2889,6 @@ static void file_transfer_operation_free(FileTransferOperation *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);
>
> spice_debug("Freeing file-transfer-operation %p", xfer_op);
> @@ -2961,7 +2959,6 @@ static void file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
> /* 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;
> }
>
> @@ -2984,7 +2981,6 @@ static void file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
>
> /* Remove and free SpiceFileTransferTask */
> g_hash_table_remove(xfer_op->xfer_task, 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));
> diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
> index a2b3885..eb943c4 100644
> --- a/src/spice-file-transfer-task.c
> +++ b/src/spice-file-transfer-task.c
> @@ -304,6 +304,7 @@ void spice_file_transfer_task_completed(SpiceFileTransferTask *self,
> self->pending = TRUE;
> signal:
> g_signal_emit(self, task_signals[SIGNAL_FINISHED], 0, self->error);
> + /* SpiceFileTransferTask unref is done after input stream is closed */
> }
>
> G_GNUC_INTERNAL
> @@ -343,9 +344,9 @@ guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask *self)
>
> /* Helper function which only creates a SpiceFileTransferTask per GFile
> * in @files and returns a HashTable mapping task-id to the task itself
> - * Note that the HashTable does not free its values upon destruction:
> - * The SpiceFileTransferTask reference created here should be freed by
> - * spice_file_transfer_task_completed */
> + * The SpiceFileTransferTask created here has two references, one should be
> + * freed by spice_file_transfer_task_completed and the other upon GHashTable
> + * destruction */
It's freed by spice_file_transfer_task_close_stream_cb after
spice_file_transfer_task_completed is called, it's not directly freed in
spice_file_transfer_task_completed. Would be nice to make the comment a
bit more descriptive.
Looks good otherwise,
Reviewed-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
Thanks,
Christophe
> G_GNUC_INTERNAL
> GHashTable *spice_file_transfer_task_create_tasks(GFile **files,
> SpiceMainChannel *channel,
> @@ -357,7 +358,7 @@ GHashTable *spice_file_transfer_task_create_tasks(GFile **files,
>
> g_return_val_if_fail(files != NULL && files[0] != NULL, NULL);
>
> - xfer_ht = g_hash_table_new(g_direct_hash, g_direct_equal);
> + xfer_ht = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_object_unref);
> for (i = 0; files[i] != NULL && !g_cancellable_is_cancelled(cancellable); i++) {
> SpiceFileTransferTask *xfer_task;
> guint32 task_id;
> @@ -374,7 +375,7 @@ GHashTable *spice_file_transfer_task_create_tasks(GFile **files,
> xfer_task->flags = flags;
>
> task_id = spice_file_transfer_task_get_id(xfer_task);
> - g_hash_table_insert(xfer_ht, GUINT_TO_POINTER(task_id), xfer_task);
> + g_hash_table_insert(xfer_ht, GUINT_TO_POINTER(task_id), g_object_ref(xfer_task));
>
> /* if we created a per-task cancellable above, unref it */
> if (!cancellable)
> --
> 2.7.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel