Re: [MM] [PATCH] iface-modem-3gpp: clear deferred registration state update when disabling

[Aleksander Morgado <aleksander lanedo com>]

On 02/27/2013 11:04 PM, Ben Chan wrote:
> ---
>  src/mm-iface-modem-3gpp.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)

When I reviewed your original patch I actually checked for when we were
removing the timeout when disabling, and assumed it was done when we set
the GObject's data to NULL, but I mixed the two data objects we handle
here, as one of them (the one where this new timeout id is stored) isn't
removed during disabling. So, yes, we need this method to remove the
timeout. Now, I would add the clearing method just when we start the
disabling sequence, next to the periodic_registration_check_disable()
call. Or was it added in CLEANUP_UNSOLICITED_REGISTRATION_EVENTS for
some reason?

Also some minor things below.

> diff --git a/src/mm-iface-modem-3gpp.c b/src/mm-iface-modem-3gpp.c
> index fa8da1a..3f3a803 100644
> --- a/src/mm-iface-modem-3gpp.c
> +++ b/src/mm-iface-modem-3gpp.c
> @@ -1325,6 +1325,18 @@ periodic_registration_check_enable (MMIfaceModem3gpp *self)
>                               (GDestroyNotify)registration_check_context_free);
>  }
>  
> +static void
> +clear_deferred_registration_state_update (MMIfaceModem3gpp *self)
> +{
> +    RegistrationStateContext *ctx = get_registration_state_context (self);
> +
> +    ctx->deferred_new_state = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
> +    if (ctx->deferred_update_id) {
> +        g_source_remove (ctx->deferred_update_id);
> +        ctx->deferred_update_id = 0;
> +    }
> +}
> +
>  /*****************************************************************************/
>  
>  typedef struct _DisablingContext DisablingContext;
> @@ -1435,7 +1447,10 @@ interface_disabling_step (DisablingContext *ctx)
>          ctx->step++;
>      }
>  
> -    case DISABLING_STEP_CLEANUP_UNSOLICITED_REGISTRATION_EVENTS:
> +    case DISABLING_STEP_CLEANUP_UNSOLICITED_REGISTRATION_EVENTS: {

No real need to add braces here, unless we're adding new variables.

> +        /* Prevent any deferred registration state update from happending after the modem is disabled */


happending doesn't seem an English word to me :)


> +        clear_deferred_registration_state_update (ctx->self);
> +
>          if (MM_IFACE_MODEM_3GPP_GET_INTERFACE (ctx->self)->cleanup_unsolicited_registration_events &&
>              MM_IFACE_MODEM_3GPP_GET_INTERFACE (ctx->self)->cleanup_unsolicited_registration_events_finish) 
> {
>              MM_IFACE_MODEM_3GPP_GET_INTERFACE (ctx->self)->cleanup_unsolicited_registration_events (
> @@ -1446,6 +1461,7 @@ interface_disabling_step (DisablingContext *ctx)
>          }
>          /* Fall down to next step */
>          ctx->step++;
> +    }
>  
>      case DISABLING_STEP_CLEANUP_UNSOLICITED_EVENTS:
>          if (MM_IFACE_MODEM_3GPP_GET_INTERFACE (ctx->self)->cleanup_unsolicited_events &&


-- 
Aleksander