[PATCH v3] bearer-mbim: disconnect attempt should succeed if bearer is already disconnected

[Dan Williams]

On Wed, 2014-06-18 at 07:59 -0700, Prathmesh Prabhu Chromium wrote:
> On Wed, Jun 18, 2014 at 7:46 AM, Dan Williams <dcbw at redhat.com> wrote:
> 
> > On Wed, 2014-06-18 at 07:24 -0700, pprabhu at chromium.org wrote:
> > > From: Prathmesh Prabhu <pprabhu at chromium.org>
> > >
> > > When trying to disconnect bearer, if the modem responds with
> > > MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED, take it to mean that the bearer
> > has
> > > already been disconnected.
> > > ---
> > >  src/mm-bearer-mbim.c | 64
> > ++++++++++++++++++++++++++++++++--------------------
> > >  1 file changed, 39 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/src/mm-bearer-mbim.c b/src/mm-bearer-mbim.c
> > > index b1e3176..2ff3415 100644
> > > --- a/src/mm-bearer-mbim.c
> > > +++ b/src/mm-bearer-mbim.c
> > > @@ -950,35 +950,49 @@ disconnect_set_ready (MbimDevice *device,
> > >      guint32 nw_error;
> > >
> > >      response = mbim_device_command_finish (device, res, &error);
> > > -    if (response &&
> > > -        (mbim_message_command_done_get_result (response, &error) ||
> > > -         error->code == MBIM_STATUS_ERROR_FAILURE)) {
> > > +    if (response) {
> > >          GError *inner_error = NULL;
> > > +        gboolean result = FALSE, parsed_result = FALSE;
> > > +
> > > +        result = mbim_message_command_done_get_result (response,
> > &error);
> > > +        /* Parse the response only for the cases we need to */
> > > +        if (result ||
> > > +            g_error_matches (error, MBIM_STATUS_ERROR,
> > MBIM_STATUS_ERROR_FAILURE) ||
> > > +            g_error_matches (error, MBIM_STATUS_ERROR,
> > > +                             MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED)) {
> > > +            parsed_result = mbim_message_connect_response_parse (
> > > +                    response,
> > > +                    &session_id,
> > > +                    &activation_state,
> > > +                    NULL, /* voice_call_state */
> > > +                    NULL, /* ip_type */
> > > +                    NULL, /* context_type */
> > > +                    &nw_error,
> > > +                    &inner_error);
> > > +        }
> > >
> > > -        if (mbim_message_connect_response_parse (
> > > -                response,
> > > -                &session_id,
> > > -                &activation_state,
> > > -                NULL, /* voice_call_state */
> > > -                NULL, /* ip_type */
> > > -                NULL, /* context_type */
> > > -                &nw_error,
> > > -                &inner_error)) {
> > > +        /* Now handle different response / error cases */
> > > +        if (parsed_result) {
> > > +            mm_dbg ("Session ID '%u': %s",
> > > +                    session_id,
> > > +                    mbim_activation_state_get_string
> > (activation_state));
> > > +        } else if (g_error_matches (error,
> > > +                                    MBIM_STATUS_ERROR,
> > > +
> >  MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED)) {
> > > +            mm_dbg ("Session ID '%u' already disconnected.",
> > session_id);
> > > +            g_clear_error (&error);
> > > +            g_clear_error (&inner_error);
> > > +        } else if (g_error_matches (error, MBIM_STATUS_ERROR,
> > MBIM_STATUS_ERROR_FAILURE)) {
> > >              if (nw_error) {
> > > -                if (error)
> > > -                    g_error_free (error);
> > > +                g_error_free (error);
> > >                  error = mm_mobile_equipment_error_from_mbim_nw_error
> > (nw_error);
> > > -            } else
> > > -                mm_dbg ("Session ID '%u': %s",
> > > -                        session_id,
> > > -                        mbim_activation_state_get_string
> > (activation_state));
> > > -        } else {
> > > -            /* Prefer the error from the result to the parsing error */
> > > -            if (!error)
> > > -                error = inner_error;
> > > -            else
> > > -                g_error_free (inner_error);
> > > -        }
> > > +            }
> > > +        }  /* else: For other errors, give precedence to error over
> > nw_error */
> > > +
> > > +        /* Give precedence to original error over parsing error */
> > > +        if (!error && inner_error)
> > > +            error = inner_error;
> > > +        g_clear_error (&inner_error);
> >
> > Oops!
> Yes, your snippet is logically correct.
> 
> 
> >  Missed this the first time around; this g_clear_error() will also clear
> > 'error' due to the pointer assignment but leave 'error' dangling.  Would
> > this:
> >
> >         if (!error && inner_error)
> >             error = inner_error;
> >         else
> >             g_clear_error (&inner_error);
> >
> > be OK?  I can fix that up when I commit if that it fine.
> >
> I think this is not a performance critical path really.
> In that case, it might be clearer to say
> 
>     if (!error && inner_error)
>         error = g_error_copy (inner_error);
>     g_clear_error (&inner_error);
> 
> You're the style czar. You decide ;)

I pushed the patch with your suggestion.

Thanks!
Dan