[PATCH 10/10] iface-modem: port mm_iface_modem_wait_for_final_state to use GTask
Ben Chan
benchan at chromium.org
Fri Jun 30 10:53:15 UTC 2017
On Fri, Jun 30, 2017 at 3:40 AM, Aleksander Morgado
<aleksander at aleksander.es> wrote:
> 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?
Yep, that's a better approach :)
>
> 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
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
More information about the ModemManager-devel
mailing list