One question inline
On Feb 28, 2013 12:23 AM, "Aleksander Morgado" <aleksander lanedo com> wrote:
>
> 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?
>
Could an unsolicited registration state update happen between invocations of interface_disabling_step()? I was wondering if the deferred_update_id should be cleared after unsolicited registration event handling is removed.
> 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 :)
>
Oops... typo :)
>
> > + 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