[PULL 02/27] io: separate freeing of tasks from marking them as complete

["Daniel P. Berrangé" <berrange@redhat.com>]

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.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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 d0929ba232..100faba6b3 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 da79d31782..c3615d2d74 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) {
@@ -108,6 +112,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;
 }
@@ -194,7 +199,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.53.0