[PATCH 1/3] io: separate freeing of tasks from marking them as complete

Daniel P. Berrangé posted 3 patches 1 month ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>
[PATCH 1/3] io: separate freeing of tasks from marking them as complete
Posted by Daniel P. Berrangé 1 month ago
The original design of QIOTask was intended to simplify lifecycle
management by automatically freeing it when the task was marked as
complete. This overlooked the fact that when a QIOTask is used in
combination with a GSource, there may be times when the source
callback is never invoked. This is typically when a GSource is
released before any I/O event arrives. In such cases it is not
desirable to mark a QIOTask as complete, but it still needs to be
freed. To satisfy this, the task must be released manually.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/io/task.h         | 29 +++++++++++++++++++++--------
 io/channel-tls.c          |  4 ++++
 io/channel-websock.c      |  3 +++
 io/task.c                 |  8 ++++++--
 tests/unit/test-io-task.c | 26 ++++++++++++++++++++++++++
 5 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/include/io/task.h b/include/io/task.h
index 0b5342ee84..98847f5994 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -96,7 +96,7 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
  *                         1000,
  *                         myobject_operation_timer,
  *                         task,
- *                         NULL);
+ *                         qio_task_free);
  *    }
  *   </programlisting>
  * </example>
@@ -138,9 +138,8 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
  * the callback func 'myobject_operation_notify' shown
  * earlier to deal with the results.
  *
- * Once this function returns false, object_unref will be called
- * automatically on the task causing it to be released and the
- * ref on QMyObject dropped too.
+ * Once this function returns FALSE, the task will be freed,
+ * causing it release the ref on QMyObject too.
  *
  * The QIOTask module can also be used to perform operations
  * in a background thread context, while still reporting the
@@ -208,8 +207,8 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
  * 'err' attribute in the task object to determine if
  * the operation was successful or not.
  *
- * The returned task will be released when qio_task_complete()
- * is invoked.
+ * The returned task must be released by calling
+ * qio_task_free() when no longer required.
  *
  * Returns: the task struct
  */
@@ -218,6 +217,19 @@ QIOTask *qio_task_new(Object *source,
                       gpointer opaque,
                       GDestroyNotify destroy);
 
+/**
+ * qio_task_free:
+ * task: the task object to free
+ *
+ * Free the resources associated with the task. Typically
+ * the qio_task_complete() method will be called immediately
+ * before this to trigger the task callback, however, it is
+ * permissible to free the task in the case of cancellation.
+ * The destroy callback will be used to release the opaque
+ * data provided to qio_task_new().
+ */
+void qio_task_free(QIOTask *task);
+
 /**
  * qio_task_run_in_thread:
  * @task: the task struct
@@ -268,8 +280,9 @@ void qio_task_wait_thread(QIOTask *task);
  * qio_task_complete:
  * @task: the task struct
  *
- * Invoke the completion callback for @task and
- * then free its memory.
+ * Invoke the completion callback for @task. This should typically
+ * only be invoked once on a task, and then qio_task_free() used
+ * to free it.
  */
 void qio_task_complete(QIOTask *task);
 
diff --git a/io/channel-tls.c b/io/channel-tls.c
index b0cec27cb9..07274c12df 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -170,6 +170,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
         trace_qio_channel_tls_handshake_fail(ioc);
         qio_task_set_error(task, err);
         qio_task_complete(task);
+        qio_task_free(task);
         return;
     }
 
@@ -183,6 +184,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
             trace_qio_channel_tls_credentials_allow(ioc);
         }
         qio_task_complete(task);
+        qio_task_free(task);
     } else {
         GIOCondition condition;
         QIOChannelTLSData *data = g_new0(typeof(*data), 1);
@@ -270,11 +272,13 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
         trace_qio_channel_tls_bye_fail(ioc);
         qio_task_set_error(task, err);
         qio_task_complete(task);
+        qio_task_free(task);
         return;
     }
 
     if (status == QCRYPTO_TLS_BYE_COMPLETE) {
         qio_task_complete(task);
+        qio_task_free(task);
         return;
     }
 
diff --git a/io/channel-websock.c b/io/channel-websock.c
index cb4dafdebb..b4f96a0af4 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -545,6 +545,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
         trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
         qio_task_set_error(task, err);
         qio_task_complete(task);
+        qio_task_free(task);
         wioc->hs_io_tag = 0;
         return FALSE;
     }
@@ -561,6 +562,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
             trace_qio_channel_websock_handshake_complete(ioc);
             qio_task_complete(task);
         }
+        qio_task_free(task);
         wioc->hs_io_tag = 0;
         return FALSE;
     }
@@ -588,6 +590,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
         trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
         qio_task_set_error(task, err);
         qio_task_complete(task);
+        qio_task_free(task);
         wioc->hs_io_tag = 0;
         return FALSE;
     }
diff --git a/io/task.c b/io/task.c
index 451f26f8b4..331febd4e1 100644
--- a/io/task.c
+++ b/io/task.c
@@ -70,8 +70,12 @@ QIOTask *qio_task_new(Object *source,
     return task;
 }
 
-static void qio_task_free(QIOTask *task)
+void qio_task_free(QIOTask *task)
 {
+    if (!task) {
+        return;
+    }
+
     qemu_mutex_lock(&task->thread_lock);
     if (task->thread) {
         if (task->thread->destroy) {
@@ -110,6 +114,7 @@ static gboolean qio_task_thread_result(gpointer opaque)
 
     trace_qio_task_thread_result(task);
     qio_task_complete(task);
+    qio_task_free(task);
 
     return FALSE;
 }
@@ -196,7 +201,6 @@ void qio_task_complete(QIOTask *task)
 {
     task->func(task, task->opaque);
     trace_qio_task_complete(task);
-    qio_task_free(task);
 }
 
 
diff --git a/tests/unit/test-io-task.c b/tests/unit/test-io-task.c
index 115dba8970..b1c8ecb7ab 100644
--- a/tests/unit/test-io-task.c
+++ b/tests/unit/test-io-task.c
@@ -73,6 +73,7 @@ static void test_task_complete(void)
     src = qio_task_get_source(task);
 
     qio_task_complete(task);
+    qio_task_free(task);
 
     g_assert(obj == src);
 
@@ -84,6 +85,28 @@ static void test_task_complete(void)
 }
 
 
+static void test_task_cancel(void)
+{
+    QIOTask *task;
+    Object *obj = object_new(TYPE_DUMMY);
+    Object *src;
+    struct TestTaskData data = { NULL, NULL, false };
+
+    task = qio_task_new(obj, task_callback, &data, NULL);
+    src = qio_task_get_source(task);
+
+    qio_task_free(task);
+
+    g_assert(obj == src);
+
+    object_unref(obj);
+
+    g_assert(data.source == NULL);
+    g_assert(data.err == NULL);
+    g_assert(data.freed == false);
+}
+
+
 static void task_data_free(gpointer opaque)
 {
     struct TestTaskData *data = opaque;
@@ -101,6 +124,7 @@ static void test_task_data_free(void)
     task = qio_task_new(obj, task_callback, &data, task_data_free);
 
     qio_task_complete(task);
+    qio_task_free(task);
 
     object_unref(obj);
 
@@ -123,6 +147,7 @@ static void test_task_failure(void)
 
     qio_task_set_error(task, err);
     qio_task_complete(task);
+    qio_task_free(task);
 
     object_unref(obj);
 
@@ -260,6 +285,7 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_QOM);
     type_register_static(&dummy_info);
     g_test_add_func("/crypto/task/complete", test_task_complete);
+    g_test_add_func("/crypto/task/cancel", test_task_cancel);
     g_test_add_func("/crypto/task/datafree", test_task_data_free);
     g_test_add_func("/crypto/task/failure", test_task_failure);
     g_test_add_func("/crypto/task/thread_complete", test_task_thread_complete);
-- 
2.52.0


Re: [PATCH 1/3] io: separate freeing of tasks from marking them as complete
Posted by Marc-André Lureau 4 weeks, 1 day ago
On Wed, Jan 7, 2026 at 7:36 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The original design of QIOTask was intended to simplify lifecycle
> management by automatically freeing it when the task was marked as
> complete. This overlooked the fact that when a QIOTask is used in
> combination with a GSource, there may be times when the source
> callback is never invoked. This is typically when a GSource is
> released before any I/O event arrives. In such cases it is not
> desirable to mark a QIOTask as complete, but it still needs to be
> freed. To satisfy this, the task must be released manually.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  include/io/task.h         | 29 +++++++++++++++++++++--------
>  io/channel-tls.c          |  4 ++++
>  io/channel-websock.c      |  3 +++
>  io/task.c                 |  8 ++++++--
>  tests/unit/test-io-task.c | 26 ++++++++++++++++++++++++++
>  5 files changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/include/io/task.h b/include/io/task.h
> index 0b5342ee84..98847f5994 100644
> --- a/include/io/task.h
> +++ b/include/io/task.h
> @@ -96,7 +96,7 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
>   *                         1000,
>   *                         myobject_operation_timer,
>   *                         task,
> - *                         NULL);
> + *                         qio_task_free);
>   *    }
>   *   </programlisting>
>   * </example>
> @@ -138,9 +138,8 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
>   * the callback func 'myobject_operation_notify' shown
>   * earlier to deal with the results.
>   *
> - * Once this function returns false, object_unref will be called
> - * automatically on the task causing it to be released and the
> - * ref on QMyObject dropped too.
> + * Once this function returns FALSE, the task will be freed,
> + * causing it release the ref on QMyObject too.
>   *
>   * The QIOTask module can also be used to perform operations
>   * in a background thread context, while still reporting the
> @@ -208,8 +207,8 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
>   * 'err' attribute in the task object to determine if
>   * the operation was successful or not.
>   *
> - * The returned task will be released when qio_task_complete()
> - * is invoked.
> + * The returned task must be released by calling
> + * qio_task_free() when no longer required.
>   *
>   * Returns: the task struct
>   */
> @@ -218,6 +217,19 @@ QIOTask *qio_task_new(Object *source,
>                        gpointer opaque,
>                        GDestroyNotify destroy);
>
> +/**
> + * qio_task_free:
> + * task: the task object to free
> + *
> + * Free the resources associated with the task. Typically
> + * the qio_task_complete() method will be called immediately
> + * before this to trigger the task callback, however, it is
> + * permissible to free the task in the case of cancellation.
> + * The destroy callback will be used to release the opaque
> + * data provided to qio_task_new().
> + */
> +void qio_task_free(QIOTask *task);
> +
>  /**
>   * qio_task_run_in_thread:
>   * @task: the task struct
> @@ -268,8 +280,9 @@ void qio_task_wait_thread(QIOTask *task);
>   * qio_task_complete:
>   * @task: the task struct
>   *
> - * Invoke the completion callback for @task and
> - * then free its memory.
> + * Invoke the completion callback for @task. This should typically
> + * only be invoked once on a task, and then qio_task_free() used
> + * to free it.
>   */
>  void qio_task_complete(QIOTask *task);
>
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index b0cec27cb9..07274c12df 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -170,6 +170,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
>          trace_qio_channel_tls_handshake_fail(ioc);
>          qio_task_set_error(task, err);
>          qio_task_complete(task);
> +        qio_task_free(task);
>          return;
>      }
>
> @@ -183,6 +184,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
>              trace_qio_channel_tls_credentials_allow(ioc);
>          }
>          qio_task_complete(task);
> +        qio_task_free(task);
>      } else {
>          GIOCondition condition;
>          QIOChannelTLSData *data = g_new0(typeof(*data), 1);
> @@ -270,11 +272,13 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
>          trace_qio_channel_tls_bye_fail(ioc);
>          qio_task_set_error(task, err);
>          qio_task_complete(task);
> +        qio_task_free(task);
>          return;
>      }
>
>      if (status == QCRYPTO_TLS_BYE_COMPLETE) {
>          qio_task_complete(task);
> +        qio_task_free(task);
>          return;
>      }
>
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index cb4dafdebb..b4f96a0af4 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -545,6 +545,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
>          trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
>          qio_task_set_error(task, err);
>          qio_task_complete(task);
> +        qio_task_free(task);
>          wioc->hs_io_tag = 0;
>          return FALSE;
>      }
> @@ -561,6 +562,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
>              trace_qio_channel_websock_handshake_complete(ioc);
>              qio_task_complete(task);
>          }
> +        qio_task_free(task);
>          wioc->hs_io_tag = 0;
>          return FALSE;
>      }
> @@ -588,6 +590,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
>          trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
>          qio_task_set_error(task, err);
>          qio_task_complete(task);
> +        qio_task_free(task);
>          wioc->hs_io_tag = 0;
>          return FALSE;
>      }
> diff --git a/io/task.c b/io/task.c
> index 451f26f8b4..331febd4e1 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -70,8 +70,12 @@ QIOTask *qio_task_new(Object *source,
>      return task;
>  }
>
> -static void qio_task_free(QIOTask *task)
> +void qio_task_free(QIOTask *task)
>  {
> +    if (!task) {
> +        return;
> +    }
> +
>      qemu_mutex_lock(&task->thread_lock);
>      if (task->thread) {
>          if (task->thread->destroy) {
> @@ -110,6 +114,7 @@ static gboolean qio_task_thread_result(gpointer opaque)
>
>      trace_qio_task_thread_result(task);
>      qio_task_complete(task);
> +    qio_task_free(task);
>
>      return FALSE;
>  }
> @@ -196,7 +201,6 @@ void qio_task_complete(QIOTask *task)
>  {
>      task->func(task, task->opaque);
>      trace_qio_task_complete(task);
> -    qio_task_free(task);
>  }
>
>
> diff --git a/tests/unit/test-io-task.c b/tests/unit/test-io-task.c
> index 115dba8970..b1c8ecb7ab 100644
> --- a/tests/unit/test-io-task.c
> +++ b/tests/unit/test-io-task.c
> @@ -73,6 +73,7 @@ static void test_task_complete(void)
>      src = qio_task_get_source(task);
>
>      qio_task_complete(task);
> +    qio_task_free(task);
>
>      g_assert(obj == src);
>
> @@ -84,6 +85,28 @@ static void test_task_complete(void)
>  }
>
>
> +static void test_task_cancel(void)
> +{
> +    QIOTask *task;
> +    Object *obj = object_new(TYPE_DUMMY);
> +    Object *src;
> +    struct TestTaskData data = { NULL, NULL, false };
> +
> +    task = qio_task_new(obj, task_callback, &data, NULL);
> +    src = qio_task_get_source(task);
> +
> +    qio_task_free(task);
> +
> +    g_assert(obj == src);
> +
> +    object_unref(obj);
> +
> +    g_assert(data.source == NULL);
> +    g_assert(data.err == NULL);
> +    g_assert(data.freed == false);
> +}
> +
> +
>  static void task_data_free(gpointer opaque)
>  {
>      struct TestTaskData *data = opaque;
> @@ -101,6 +124,7 @@ static void test_task_data_free(void)
>      task = qio_task_new(obj, task_callback, &data, task_data_free);
>
>      qio_task_complete(task);
> +    qio_task_free(task);
>
>      object_unref(obj);
>
> @@ -123,6 +147,7 @@ static void test_task_failure(void)
>
>      qio_task_set_error(task, err);
>      qio_task_complete(task);
> +    qio_task_free(task);
>
>      object_unref(obj);
>
> @@ -260,6 +285,7 @@ int main(int argc, char **argv)
>      module_call_init(MODULE_INIT_QOM);
>      type_register_static(&dummy_info);
>      g_test_add_func("/crypto/task/complete", test_task_complete);
> +    g_test_add_func("/crypto/task/cancel", test_task_cancel);
>      g_test_add_func("/crypto/task/datafree", test_task_data_free);
>      g_test_add_func("/crypto/task/failure", test_task_failure);
>      g_test_add_func("/crypto/task/thread_complete", test_task_thread_complete);
> --
> 2.52.0
>
>


-- 
Marc-André Lureau