[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