[PATCH] libmbim-glib,device: port transactions to GTask
Ben Chan
benchan at chromium.org
Wed Aug 23 23:18:50 UTC 2017
On Sun, Aug 20, 2017 at 2:10 AM, Aleksander Morgado <
aleksander at aleksander.es> wrote:
> ---
> src/libmbim-glib/mbim-device.c | 389 ++++++++++++++++++++++--------
> -----------
> 1 file changed, 207 insertions(+), 182 deletions(-)
>
> diff --git a/src/libmbim-glib/mbim-device.c b/src/libmbim-glib/mbim-
> device.c
> index 4b7d431..082f081 100644
> --- a/src/libmbim-glib/mbim-device.c
> +++ b/src/libmbim-glib/mbim-device.c
> @@ -144,140 +144,155 @@ static void device_report_error (MbimDevice
> *self,
> /* Message transactions (private) */
>
> typedef struct {
> - MbimDevice *self;
> - guint32 transaction_id;
> - TransactionType type;
> + MbimDevice *self;
> + guint32 transaction_id;
> + TransactionType type;
> } TransactionWaitContext;
>
> typedef struct {
> - MbimDevice *self;
> - MbimMessage *fragments;
> - MbimMessageType type;
> - guint32 transaction_id;
> - GSimpleAsyncResult *result;
> - GSource *timeout_source;
> - GCancellable *cancellable;
> - gulong cancellable_id;
> + MbimMessage *fragments;
> + MbimMessageType type;
> + guint32 transaction_id;
> + GSource *timeout_source;
> + GCancellable *cancellable;
> + gulong cancellable_id;
> TransactionWaitContext *wait_ctx;
> -} Transaction;
> +} TransactionContext;
> +
> +static void
> +transaction_context_free (TransactionContext *ctx)
> +{
> + if (ctx->fragments)
> + mbim_message_unref (ctx->fragments);
> +
> + if (ctx->timeout_source)
> + g_source_destroy (ctx->timeout_source);
> +
> + if (ctx->cancellable) {
>
[Ben] Looks like ctx->cancellable is never assigned.
> + if (ctx->cancellable_id)
> + g_cancellable_disconnect (ctx->cancellable,
> ctx->cancellable_id);
> + g_object_unref (ctx->cancellable);
> + }
> +
> + if (ctx->wait_ctx)
> + g_slice_free (TransactionWaitContext, ctx->wait_ctx);
> +
> + g_slice_free (TransactionContext, ctx);
> +}
>
> /* #define TRACE_TRANSACTION 1 */
> #ifdef TRACE_TRANSACTION
> static void
> -trace_transaction (Transaction *tr,
> - const gchar *state)
> +transaction_task_trace (GTask *task,
> + const gchar *state)
> {
> + MbimDevice *self;
> + TransactionContext *ctx;
> +
> + self = g_task_get_source_object (task);
> + ctx = g_task_get_task_data (task);
> +
> g_debug ("[%s,%u] transaction %s: %s",
> - tr->self->priv->path_display,
> - tr->transaction_id,
> - mbim_message_type_get_string (tr->type),
> + self->priv->path_display,
> + ctx->transaction_id,
> + mbim_message_type_get_string (ctx->type),
> state);
> }
> #else
> -# define trace_transaction(...)
> +# define transaction_task_trace(...)
> #endif
>
> -static Transaction *
> -transaction_new (MbimDevice *self,
> - MbimMessageType type,
> - guint32 transaction_id,
> - GCancellable *cancellable,
> - GAsyncReadyCallback callback,
> - gpointer user_data)
> +static GTask *
> +transaction_task_new (MbimDevice *self,
> + MbimMessageType type,
> + guint32 transaction_id,
> + GCancellable *cancellable,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> {
> - Transaction *tr;
> + GTask *task;
> + TransactionContext *ctx;
>
> - tr = g_slice_new0 (Transaction);
> - tr->type = type;
> - tr->transaction_id = transaction_id;
> - tr->self = g_object_ref (self);
> - tr->result = g_simple_async_result_new (G_OBJECT (self),
> - callback,
> - user_data,
> - transaction_new);
> - if (cancellable)
> - tr->cancellable = g_object_ref (cancellable);
> + task = g_task_new (self, cancellable, callback, user_data);
> +
> + ctx = g_slice_new0 (TransactionContext);
> + ctx->type = type;
> + ctx->transaction_id = transaction_id;
> + g_task_set_task_data (task, ctx, (GDestroyNotify)
> transaction_context_free);
>
> - trace_transaction (tr, "new");
> + transaction_task_trace (task, "new");
>
> - return tr;
> + return task;
> }
>
> static void
> -transaction_complete_and_free (Transaction *tr,
> - const GError *error)
> +transaction_task_complete_and_free (GTask *task,
> + const GError *error)
> {
> - if (tr->timeout_source)
> - g_source_destroy (tr->timeout_source);
> -
> - if (tr->cancellable) {
> - if (tr->cancellable_id)
> - g_cancellable_disconnect (tr->cancellable,
> tr->cancellable_id);
> - g_object_unref (tr->cancellable);
> - }
> + TransactionContext *ctx;
>
> - if (tr->wait_ctx)
> - g_slice_free (TransactionWaitContext, tr->wait_ctx);
> + ctx = g_task_get_task_data (task);
>
> if (error) {
> - trace_transaction (tr, "complete: error");
> - g_simple_async_result_set_from_error (tr->result, error);
> - if (tr->fragments)
> - mbim_message_unref (tr->fragments);
> + transaction_task_trace (task, "complete: error");
> + g_task_return_error (task, g_error_copy (error));
> } else {
> - trace_transaction (tr, "complete: response");
> - g_assert (tr->fragments != NULL);
> - g_simple_async_result_set_op_res_gpointer (tr->result,
> - tr->fragments,
> - (GDestroyNotify)
> mbim_message_unref);
> + transaction_task_trace (task, "complete: response");
> + g_assert (ctx->fragments != NULL);
> + g_task_return_pointer (task, mbim_message_ref (ctx->fragments),
> (GDestroyNotify) mbim_message_unref);
> }
>
> - g_simple_async_result_complete_in_idle (tr->result);
> - g_object_unref (tr->result);
> - g_object_unref (tr->self);
> - g_slice_free (Transaction, tr);
> + g_object_unref (task);
> }
>
> -static Transaction *
> +static GTask *
> device_release_transaction (MbimDevice *self,
> TransactionType type,
> MbimMessageType expected_type,
> guint32 transaction_id)
> {
> - Transaction *tr = NULL;
> + GTask *task;
> + TransactionContext *ctx;
>
> /* Only return transaction if it was released from the HT */
> - if (self->priv->transactions[type]) {
> - tr = g_hash_table_lookup (self->priv->transactions[type],
> GUINT_TO_POINTER (transaction_id));
> - if (tr && ((tr->type == expected_type) || (expected_type ==
> MBIM_MESSAGE_TYPE_INVALID))) {
> - /* If found, remove it from the HT */
> - trace_transaction (tr, "release");
> - g_hash_table_remove (self->priv->transactions[type],
> GUINT_TO_POINTER (transaction_id));
> - return tr;
> - }
> + if (!self->priv->transactions[type])
> + return NULL;
> +
> + task = g_hash_table_lookup (self->priv->transactions[type],
> GUINT_TO_POINTER (transaction_id));
> + if (!task)
> + return NULL;
> +
> + ctx = g_task_get_task_data (task);
> + if ((ctx->type == expected_type) || (expected_type ==
> MBIM_MESSAGE_TYPE_INVALID)) {
> + /* If found, remove it from the HT */
> + transaction_task_trace (task, "release");
> + g_hash_table_remove (self->priv->transactions[type],
> GUINT_TO_POINTER (transaction_id));
> + return task;
> }
>
> return NULL;
> }
>
> static gboolean
> -transaction_timed_out (TransactionWaitContext *ctx)
> +transaction_timed_out (TransactionWaitContext *wait_ctx)
> {
> - Transaction *tr;
> - GError *error = NULL;
> + GTask *task;
> + TransactionContext *ctx;
> + GError *error = NULL;
>
> - tr = device_release_transaction (ctx->self,
> - ctx->type,
> - MBIM_MESSAGE_TYPE_INVALID,
> - ctx->transaction_id);
> - if (!tr)
> + task = device_release_transaction (wait_ctx->self,
> + wait_ctx->type,
> + MBIM_MESSAGE_TYPE_INVALID,
> + wait_ctx->transaction_id);
> + if (!task)
> /* transaction already completed */
> return FALSE;
>
> - tr->timeout_source = NULL;
> + ctx = g_task_get_task_data (task);
> + ctx->timeout_source = NULL;
>
> /* If no fragment was received, complete transaction with a timeout
> error */
> - if (!tr->fragments)
> + if (!ctx->fragments)
> error = g_error_new (MBIM_CORE_ERROR,
> MBIM_CORE_ERROR_TIMEOUT,
> "Transaction timed out");
> @@ -288,78 +303,87 @@ transaction_timed_out (TransactionWaitContext *ctx)
> "Fragment timed out");
>
> /* Also notify to the modem */
> - device_report_error (ctx->self,
> - tr->transaction_id,
> + device_report_error (wait_ctx->self,
> + wait_ctx->transaction_id,
> error);
> }
>
> - transaction_complete_and_free (tr, error);
> + transaction_task_complete_and_free (task, error);
> g_error_free (error);
>
> - return FALSE;
> + return G_SOURCE_REMOVE;
> }
>
> static void
> transaction_cancelled (GCancellable *cancellable,
> - TransactionWaitContext *ctx)
> + TransactionWaitContext *wait_ctx)
> {
> - Transaction *tr;
> - GError *error = NULL;
> + GTask *task;
> + TransactionContext *ctx;
> + GError *error = NULL;
>
> - tr = device_release_transaction (ctx->self,
> - ctx->type,
> - MBIM_MESSAGE_TYPE_INVALID,
> - ctx->transaction_id);
> + task = device_release_transaction (wait_ctx->self,
> + wait_ctx->type,
> + MBIM_MESSAGE_TYPE_INVALID,
> + wait_ctx->transaction_id);
>
> /* The transaction may have already been cancelled before we stored
> it in
> * the tracking table */
> - if (!tr)
> + if (!task)
> return;
>
> - tr->cancellable_id = 0;
> + ctx = g_task_get_task_data (task);
> + ctx->cancellable_id = 0;
>
> /* Complete transaction with an abort error */
> error = g_error_new (MBIM_CORE_ERROR,
> MBIM_CORE_ERROR_ABORTED,
> "Transaction aborted");
> - transaction_complete_and_free (tr, error);
> + transaction_task_complete_and_free (task, error);
> g_error_free (error);
> }
>
> static gboolean
> device_store_transaction (MbimDevice *self,
> TransactionType type,
> - Transaction *tr,
> + GTask *task,
> guint timeout_ms,
> GError **error)
> {
> - trace_transaction (tr, "store");
> + TransactionContext *ctx;
> + GCancellable *cancellable;
> +
> + transaction_task_trace (task, "store");
>
> if (G_UNLIKELY (!self->priv->transactions[type]))
> self->priv->transactions[type] = g_hash_table_new (g_direct_hash,
> g_direct_equal);
>
> - tr->wait_ctx = g_slice_new (TransactionWaitContext);
> - tr->wait_ctx->self = self;
> + ctx = g_task_get_task_data (task);
> +
> + ctx->wait_ctx = g_slice_new (TransactionWaitContext);
> + ctx->wait_ctx->self = self;
> /* valid as long as the transaction is in the HT */
> - tr->wait_ctx->transaction_id = tr->transaction_id;
> - tr->wait_ctx->type = type;
> + ctx->wait_ctx->transaction_id = ctx->transaction_id;
> + ctx->wait_ctx->type = type;
>
> /* don't add timeout if one already exists */
> - if (!tr->timeout_source) {
> - tr->timeout_source = g_timeout_source_new (timeout_ms);
> - g_source_set_callback (tr->timeout_source,
> (GSourceFunc)transaction_timed_out, tr->wait_ctx, NULL);
> - g_source_attach (tr->timeout_source, g_main_context_get_thread_default
> ());
> - g_source_unref (tr->timeout_source);
> + if (!ctx->timeout_source) {
> + ctx->timeout_source = g_timeout_source_new (timeout_ms);
> + g_source_set_callback (ctx->timeout_source,
> (GSourceFunc)transaction_timed_out, ctx->wait_ctx, NULL);
> + g_source_attach (ctx->timeout_source, g_main_context_get_thread_default
> ());
> + g_source_unref (ctx->timeout_source);
> }
>
> - if (tr->cancellable && !tr->cancellable_id) {
> + /* Indication transactions don't have cancellable */
> + cancellable = g_task_get_cancellable (task);
> + if (cancellable && !ctx->cancellable_id) {
> /* Note: transaction_cancelled() will also be called directly if
> the
> * cancellable is already cancelled */
> - tr->cancellable_id = g_cancellable_connect (tr->cancellable,
> -
> (GCallback)transaction_cancelled,
> - tr->wait_ctx,
> - NULL);
> - if (!tr->cancellable_id) {
> + ctx->cancellable_id = g_cancellable_connect (cancellable,
> +
> (GCallback)transaction_cancelled,
> + ctx->wait_ctx,
> + NULL);
> + if (!ctx->cancellable_id) {
> g_set_error_literal (error,
> MBIM_CORE_ERROR,
> MBIM_CORE_ERROR_ABORTED,
> @@ -369,7 +393,7 @@ device_store_transaction (MbimDevice *self,
> }
>
> /* Keep in the HT */
> - g_hash_table_insert (self->priv->transactions[type],
> GUINT_TO_POINTER (tr->transaction_id), tr);
> + g_hash_table_insert (self->priv->transactions[type],
> GUINT_TO_POINTER (ctx->transaction_id), task);
>
> return TRUE;
> }
> @@ -472,7 +496,7 @@ indication_ready (MbimDevice *self,
> GError *error = NULL;
> MbimMessage *indication;
>
> - if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT
> (res), &error)) {
> + if (!(indication = g_task_propagate_pointer (G_TASK (res), &error))) {
> g_debug ("[%s] Error processing indication message: %s",
> self->priv->path_display,
> error->message);
> @@ -480,8 +504,8 @@ indication_ready (MbimDevice *self,
> return;
> }
>
> - indication = g_simple_async_result_get_op_res_gpointer
> (G_SIMPLE_ASYNC_RESULT (res));
> g_signal_emit (self, signals[SIGNAL_INDICATE_STATUS], 0, indication);
> + mbim_message_unref (indication);
> }
>
> static void
> @@ -523,32 +547,33 @@ process_message (MbimDevice *self,
> case MBIM_MESSAGE_TYPE_CLOSE_DONE:
> case MBIM_MESSAGE_TYPE_COMMAND_DONE:
> case MBIM_MESSAGE_TYPE_INDICATE_STATUS: {
> - GError *error = NULL;
> - Transaction *tr;
> + GError *error = NULL;
> + GTask *task;
> + TransactionContext *ctx;
>
[Ben] we may as well do `ctx = g_task_get_task_data (task);` here instead
of the two assignments below.
>
> if (MBIM_MESSAGE_GET_MESSAGE_TYPE (message) ==
> MBIM_MESSAGE_TYPE_INDICATE_STATUS) {
> /* Grab transaction */
> - tr = device_release_transaction (self,
> - TRANSACTION_TYPE_MODEM,
> - MBIM_MESSAGE_TYPE_INDICATE_
> STATUS,
> - mbim_message_get_transaction_id
> (message));
> + task = device_release_transaction (self,
> + TRANSACTION_TYPE_MODEM,
> + MBIM_MESSAGE_TYPE_INDICATE_
> STATUS,
> +
> mbim_message_get_transaction_id (message));
>
> - if (!tr)
> + if (!task)
> /* Create new transaction for the indication */
> - tr = transaction_new (self,
> - MBIM_MESSAGE_TYPE_INDICATE_STATUS,
> - mbim_message_get_transaction_id
> (message),
> - NULL, /* no cancellable */
> - (GAsyncReadyCallback)
> indication_ready,
> - NULL);
> + task = transaction_task_new (self,
> + MBIM_MESSAGE_TYPE_INDICATE_
> STATUS,
> + mbim_message_get_transaction_id
> (message),
> + NULL, /* no cancellable */
> + (GAsyncReadyCallback)
> indication_ready,
> + NULL);
> } else {
> /* Grab transaction. This is a _DONE message, so look for the
> request
> * that generated the _DONE */
> - tr = device_release_transaction (self,
> - TRANSACTION_TYPE_HOST,
> -
> (MBIM_MESSAGE_GET_MESSAGE_TYPE (message) - 0x80000000),
> - mbim_message_get_transaction_id
> (message));
> - if (!tr) {
> + task = device_release_transaction (self,
> + TRANSACTION_TYPE_HOST,
> +
> (MBIM_MESSAGE_GET_MESSAGE_TYPE (message) - 0x80000000),
> +
> mbim_message_get_transaction_id (message));
> + if (!task) {
> gchar *printable;
>
> g_debug ("[%s] No transaction matched in received
> message",
> @@ -566,65 +591,65 @@ process_message (MbimDevice *self,
>
> /* If the message doesn't have fragments, we're done */
> if (!_mbim_message_is_fragment (message)) {
> - g_assert (tr->fragments == NULL);
> - tr->fragments = mbim_message_dup (message);
> - transaction_complete_and_free (tr, NULL);
> + ctx = g_task_get_task_data (task);
> + g_assert (ctx->fragments == NULL);
> + ctx->fragments = mbim_message_dup (message);
> + transaction_task_complete_and_free (task, NULL);
> return;
> }
> }
>
> /* More than one fragment expected; is this the first one? */
> - if (!tr->fragments)
> - tr->fragments = _mbim_message_fragment_collector_init
> (message, &error);
> + ctx = g_task_get_task_data (task);
> + if (!ctx->fragments)
> + ctx->fragments = _mbim_message_fragment_collector_init
> (message, &error);
> else
> - _mbim_message_fragment_collector_add (tr->fragments,
> message, &error);
> + _mbim_message_fragment_collector_add (ctx->fragments,
> message, &error);
>
> if (error) {
> - device_report_error (self,
> - tr->transaction_id,
> - error);
> - transaction_complete_and_free (tr, error);
> + device_report_error (self, ctx->transaction_id, error);
> + transaction_task_complete_and_free (task, error);
> g_error_free (error);
> return;
> }
>
> /* Did we get all needed fragments? */
> - if (_mbim_message_fragment_collector_complete (tr->fragments)) {
> + if (_mbim_message_fragment_collector_complete (ctx->fragments)) {
> /* Now, translate the whole message */
> if (mbim_utils_get_traces_enabled ()) {
> gchar *printable;
>
> - printable = mbim_message_get_printable (tr->fragments,
> ">>>>>> ", FALSE);
> + printable = mbim_message_get_printable (ctx->fragments,
> ">>>>>> ", FALSE);
> g_debug ("[%s] Received message (translated)...\n%s",
> self->priv->path_display,
> printable);
> g_free (printable);
> }
>
> - transaction_complete_and_free (tr, NULL);
> + transaction_task_complete_and_free (task, NULL);
> return;
> }
>
> /* Need more fragments, store transaction */
> g_assert (device_store_transaction (self,
> TRANSACTION_TYPE_HOST,
> - tr,
> + task,
> MAX_TIME_BETWEEN_FRAGMENTS_MS,
> NULL));
> return;
> }
>
> case MBIM_MESSAGE_TYPE_FUNCTION_ERROR: {
> - Transaction *tr;
> GError *error_indication;
> + GTask *task;
>
> /* Try to match this transaction just per transaction ID */
> - tr = device_release_transaction (self,
> - TRANSACTION_TYPE_HOST,
> - MBIM_MESSAGE_TYPE_INVALID,
> - mbim_message_get_transaction_id
> (message));
> + task = device_release_transaction (self,
> + TRANSACTION_TYPE_HOST,
> + MBIM_MESSAGE_TYPE_INVALID,
> + mbim_message_get_transaction_id
> (message));
>
> - if (!tr)
> + if (!task)
> g_debug ("[%s] No transaction matched in received function
> error message",
> self->priv->path_display);
>
> @@ -638,11 +663,15 @@ process_message (MbimDevice *self,
> g_free (printable);
> }
>
> - if (tr) {
> - if (tr->fragments)
> - mbim_message_unref (tr->fragments);
> - tr->fragments = mbim_message_dup (message);
> - transaction_complete_and_free (tr, NULL);
> + if (task) {
> + TransactionContext *ctx;
> +
> + ctx = g_task_get_task_data (task);
> +
> + if (ctx->fragments)
> + mbim_message_unref (ctx->fragments);
> + ctx->fragments = mbim_message_dup (message);
> + transaction_task_complete_and_free (task, NULL);
> }
>
> /* Signals are emitted regardless of whether the transaction
> matched or not */
> @@ -1983,11 +2012,7 @@ mbim_device_command_finish (MbimDevice *self,
> GAsyncResult *res,
> GError **error)
> {
> - if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT
> (res), error))
> - return NULL;
> -
> - return mbim_message_ref (g_simple_async_result_get_op_res_gpointer (
> - G_SIMPLE_ASYNC_RESULT (res)));
> + return g_task_propagate_pointer (G_TASK (res), error);
> }
>
> /**
> @@ -2012,9 +2037,9 @@ mbim_device_command (MbimDevice *self,
> GAsyncReadyCallback callback,
> gpointer user_data)
> {
> - GError *error = NULL;
> - Transaction *tr;
> - guint32 transaction_id;
> + GError *error = NULL;
> + GTask *task;
> + guint32 transaction_id;
>
> g_return_if_fail (MBIM_IS_DEVICE (self));
> g_return_if_fail (message != NULL);
> @@ -2027,38 +2052,38 @@ mbim_device_command (MbimDevice *self,
> mbim_message_set_transaction_id (message, transaction_id);
> }
>
> - tr = transaction_new (self,
> - MBIM_MESSAGE_GET_MESSAGE_TYPE (message),
> - transaction_id,
> - cancellable,
> - callback,
> - user_data);
> + task = transaction_task_new (self,
> + MBIM_MESSAGE_GET_MESSAGE_TYPE (message),
> + transaction_id,
> + cancellable,
> + callback,
> + user_data);
>
> /* Device must be open */
> if (!self->priv->iochannel) {
> error = g_error_new (MBIM_CORE_ERROR,
> MBIM_CORE_ERROR_WRONG_STATE,
> "Device must be open to send commands");
> - transaction_complete_and_free (tr, error);
> + transaction_task_complete_and_free (task, error);
> g_error_free (error);
> return;
> }
>
> /* Setup context to match response */
> - if (!device_store_transaction (self, TRANSACTION_TYPE_HOST, tr,
> timeout * 1000, &error)) {
> + if (!device_store_transaction (self, TRANSACTION_TYPE_HOST, task,
> timeout * 1000, &error)) {
> g_prefix_error (&error, "Cannot store transaction: ");
> - transaction_complete_and_free (tr, error);
> + transaction_task_complete_and_free (task, error);
> g_error_free (error);
> return;
> }
>
> if (!device_send (self, message, &error)) {
> /* Match transaction so that we remove it from our tracking table
> */
> - tr = device_release_transaction (self,
> - TRANSACTION_TYPE_HOST,
> - MBIM_MESSAGE_GET_MESSAGE_TYPE
> (message),
> - mbim_message_get_transaction_id
> (message));
> - transaction_complete_and_free (tr, error);
> + task = device_release_transaction (self,
> + TRANSACTION_TYPE_HOST,
> + MBIM_MESSAGE_GET_MESSAGE_TYPE
> (message),
> + mbim_message_get_transaction_id
> (message));
> + transaction_task_complete_and_free (task, error);
> g_error_free (error);
> return;
> }
> --
> 2.14.1
>
> _______________________________________________
> libmbim-devel mailing list
> libmbim-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libmbim-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/libmbim-devel/attachments/20170823/b489e343/attachment-0001.html>
More information about the libmbim-devel
mailing list