[Spice-devel] [spice-gtk v2 7/9] file-transfer: increase reference for channel-main

[Christophe Fergeau]

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 at redhat.com>

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160803/cedafdf5/attachment.sig>