io: separate freeing of tasks from marking them as complete

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>

diff --git a/include/io/task.h b/include/io/task.h
index 0b5342ee8432ff8a841cb210ed6bdfbf8aae4d5f..98847f5994de2013d7752aa96b78d149d778ee2a 100644 (file)
--- 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 b0cec27cb9c0d42e3165c378038df847eb357e89..07274c12dfb67a7ecf5a7323cdb9d53cbd4f9fc8 100644 (file)
--- 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 d0929ba23269c7c97126e4cf25d0b1659ed09049..100faba6b3d85eda7d09f3a76b1d2c7ed0ae0bdb 100644 (file)
--- 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 da79d3178264d19f1c0076d79bccb826e5a9c12c..c3615d2d74fcc23fb969b774083b8181ba441853 100644 (file)
--- 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 115dba89702bc660c62956a9579b011fc5a5621a..b1c8ecb7abbdeba02702dc090f89db50996c9f2c 100644 (file)
--- 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);