[PATCH 10/10] iface-modem: port mm_iface_modem_wait_for_final_state to use GTask
Aleksander Morgado
aleksander at aleksander.es
Fri Jun 30 10:40:50 UTC 2017
Hey Ben,
See comments below.
On Fri, Jun 30, 2017 at 11:23 AM, Ben Chan <benchan at chromium.org> wrote:
> ---
> src/mm-iface-modem.c | 114 +++++++++++++++++++++++++++++++++------------------
> 1 file changed, 75 insertions(+), 39 deletions(-)
>
> diff --git a/src/mm-iface-modem.c b/src/mm-iface-modem.c
> index 586f5565..9e954cab 100644
> --- a/src/mm-iface-modem.c
> +++ b/src/mm-iface-modem.c
> @@ -89,27 +89,65 @@ mm_iface_modem_bind_simple_status (MMIfaceModem *self,
> state == MM_MODEM_STATE_CONNECTING)
>
> typedef struct {
> - MMIfaceModem *self;
> MMModemState final_state;
> - GSimpleAsyncResult *result;
> gulong state_changed_id;
> guint state_changed_wait_id;
> } WaitForFinalStateContext;
>
> +/* As g_task_propagate_int returns -1 on error and MM_MODEM_STATE_FAILED has
> + * value -1, MM_MODEM_STATE_FAILED is encoded as G_MAXSIZE when being returned
> + * via g_task_return_int.
> + */
> +static gssize
> +encode_state(MMModemState state)
> +{
> + return state == MM_MODEM_STATE_FAILED ? G_MAXSIZE : state;
> +}
> +
> +static MMModemState
> +decode_state(gssize value)
> +{
> + return value == G_MAXSIZE ? MM_MODEM_STATE_UNKNOWN : (MMModemState)value;
I believe that should have been:
return value == G_MAXSIZE ? MM_MODEM_STATE_FAILED : (MMModemState)value;
But not sure about this approach, see the other comment below.
> +}
> +
> static void
> -wait_for_final_state_context_complete_and_free (WaitForFinalStateContext *ctx)
> +wait_for_final_state_context_free (WaitForFinalStateContext *ctx)
> {
> - /* The callback associated with 'ctx->result' may update the modem state.
> + g_slice_free (WaitForFinalStateContext, ctx);
> +}
> +
> +static void
> +wait_for_final_state_context_complete (GTask *task,
> + MMModemState state,
> + GError* error)
> +{
> + MMIfaceModem *self;
> + WaitForFinalStateContext *ctx;
> +
> + self = g_task_get_source_object (task);
> + ctx = g_task_get_task_data (task);
> +
> + /* The callback associated with 'task' may update the modem state.
> * Disconnect the signal handler for modem state changes before completing
> - * 'ctx->result' in order to prevent state_changed from being invoked, which
> - * invokes wait_for_final_state_context_complete_and_free (ctx) again. */
> - g_signal_handler_disconnect (ctx->self, ctx->state_changed_id);
> + * 'task' in order to prevent state_changed from being invoked, which
> + * invokes wait_for_final_state_context_complete again. */
> + if (ctx->state_changed_id) {
> + g_signal_handler_disconnect (self, ctx->state_changed_id);
> + ctx->state_changed_id = 0;
> + }
>
> - g_simple_async_result_complete (ctx->result);
> - g_object_unref (ctx->result);
> - g_source_remove (ctx->state_changed_wait_id);
> - g_object_unref (ctx->self);
> - g_slice_free (WaitForFinalStateContext, ctx);
> + /* Remove any outstanding timeout on waiting for state change. */
> + if (ctx->state_changed_wait_id) {
> + g_source_remove (ctx->state_changed_wait_id);
> + ctx->state_changed_wait_id = 0;
> + }
> +
> + if (error)
> + g_task_return_error (task, error);
> + else
> + g_task_return_int (task, encode_state (state));
> +
> + g_object_unref (task);
> }
>
> MMModemState
> @@ -117,29 +155,30 @@ mm_iface_modem_wait_for_final_state_finish (MMIfaceModem *self,
> GAsyncResult *res,
> GError **error)
> {
> - if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error))
> - return MM_MODEM_STATE_UNKNOWN;
> + gssize value;
>
> - return (MMModemState)GPOINTER_TO_UINT (g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (res)));
> + value = g_task_propagate_int (G_TASK (res), error);
> + return value < 0 ? MM_MODEM_STATE_UNKNOWN : decode_state (value);
Instead of relying on the returned int to see if we got an error, I
think we should instead check the GError itself. If we ever provided
more fine grained state values they will all be < 0 as well, so we may
end up needing to update this logic again.
How about something like this instead?
static MMModemState
mm_iface_modem_wait_for_final_state_finish (
MMIfaceModem *self,
GAsyncResult *res,
GError **error)
{
GError *inner_error = NULL;
gssize value;
value = g_task_propagate_int (G_TASK (res), &inner_error);
if (inner_error) {
g_propagate_error (error, inner_error);
return MM_MODEM_STATE_UNKNOWN;
}
return (MMModemState) value;
}
> }
>
> static gboolean
> -state_changed_wait_expired (WaitForFinalStateContext *ctx)
> -{
> - g_simple_async_result_set_error (
> - ctx->result,
> - MM_CORE_ERROR,
> - MM_CORE_ERROR_RETRY,
> - "Too much time waiting to get to a final state");
> - wait_for_final_state_context_complete_and_free (ctx);
> +state_changed_wait_expired (GTask *task)
> +{
> + GError *error;
> +
> + error = g_error_new (MM_CORE_ERROR,
> + MM_CORE_ERROR_RETRY,
> + "Too much time waiting to get to a final state");
> + wait_for_final_state_context_complete (task, MM_MODEM_STATE_UNKNOWN, error);
> return G_SOURCE_REMOVE;
> }
>
> static void
> state_changed (MMIfaceModem *self,
> GParamSpec *spec,
> - WaitForFinalStateContext *ctx)
> + GTask *task)
> {
> + WaitForFinalStateContext *ctx;
> MMModemState state = MM_MODEM_STATE_UNKNOWN;
>
> g_object_get (self,
> @@ -150,6 +189,8 @@ state_changed (MMIfaceModem *self,
> if (MODEM_STATE_IS_INTERMEDIATE (state))
> return;
>
> + ctx = g_task_get_task_data (task);
> +
> /* If we want a specific final state and this is not the one we were
> * looking for, then skip */
> if (ctx->final_state != MM_MODEM_STATE_UNKNOWN &&
> @@ -158,8 +199,7 @@ state_changed (MMIfaceModem *self,
> return;
>
> /* Done! */
> - g_simple_async_result_set_op_res_gpointer (ctx->result, GUINT_TO_POINTER (state), NULL);
> - wait_for_final_state_context_complete_and_free (ctx);
> + wait_for_final_state_context_complete (task, state, NULL);
> }
>
> void
> @@ -170,12 +210,9 @@ mm_iface_modem_wait_for_final_state (MMIfaceModem *self,
> {
> MMModemState state = MM_MODEM_STATE_UNKNOWN;
> WaitForFinalStateContext *ctx;
> - GSimpleAsyncResult *result;
> + GTask *task;
>
> - result = g_simple_async_result_new (G_OBJECT (self),
> - callback,
> - user_data,
> - mm_iface_modem_wait_for_final_state);
> + task = g_task_new (self, NULL, callback, user_data);
>
> g_object_get (self,
> MM_IFACE_MODEM_STATE, &state,
> @@ -186,9 +223,8 @@ mm_iface_modem_wait_for_final_state (MMIfaceModem *self,
> /* Is this the state we actually wanted? */
> if (final_state == MM_MODEM_STATE_UNKNOWN ||
> (state != MM_MODEM_STATE_UNKNOWN && state == final_state)) {
> - g_simple_async_result_set_op_res_gpointer (result, GUINT_TO_POINTER (state), NULL);
> - g_simple_async_result_complete_in_idle (result);
> - g_object_unref (result);
> + g_task_return_int (task, encode_state (state));
> + g_object_unref (task);
> return;
> }
>
> @@ -196,19 +232,19 @@ mm_iface_modem_wait_for_final_state (MMIfaceModem *self,
> }
>
> ctx = g_slice_new0 (WaitForFinalStateContext);
> - ctx->result = result;
> - ctx->self = g_object_ref (self);
> ctx->final_state = final_state;
>
> + g_task_set_task_data (task, ctx, (GDestroyNotify)wait_for_final_state_context_free);
> +
> /* Want to get notified when modem state changes */
> - ctx->state_changed_id = g_signal_connect (ctx->self,
> + ctx->state_changed_id = g_signal_connect (self,
> "notify::" MM_IFACE_MODEM_STATE,
> G_CALLBACK (state_changed),
> - ctx);
> + task);
> /* But we don't want to wait forever */
> ctx->state_changed_wait_id = g_timeout_add_seconds (10,
> (GSourceFunc)state_changed_wait_expired,
> - ctx);
> + task);
> }
>
> /*****************************************************************************/
> --
> 2.13.2
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list